diff --git a/rust/bin-shared/benches/tunnel.rs b/rust/bin-shared/benches/tunnel.rs index c91f48f6b..bc71a3ad1 100644 --- a/rust/bin-shared/benches/tunnel.rs +++ b/rust/bin-shared/benches/tunnel.rs @@ -91,6 +91,7 @@ mod platform { original_udp.get_source(), vec![RESP_CODE], ) + .unwrap() }) .packet(); tun.write4(res_buf)?; diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index cea34f27e..13f9297f3 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -261,13 +261,18 @@ pub struct ClientState { /// /// The [`Instant`] tracks when the DNS query expires. mangled_dns_queries: HashMap, - /// DNS queries that were forwarded to an upstream server. + /// DNS queries that were forwarded to an upstream server, indexed by the DNS query ID + the server we sent it to. + /// + /// The value is a tuple of: /// /// - The [`SocketAddr`] is the original source IP. /// - The [`Instant`] tracks when the DNS query expires. /// /// We store an explicit expiry to avoid a memory leak in case of a non-responding DNS server. - forwarded_dns_queries: HashMap, + /// + /// DNS query IDs don't appear to be unique across servers they are being sent to on some operating systems (looking at you, Windows). + /// Hence, we need to index by ID + socket of the DNS server. + forwarded_dns_queries: HashMap<(u16, SocketAddr), (SocketAddr, Instant)>, /// Manages internal dns records and emits forwarding event when not internally handled stub_resolver: StubResolver, @@ -649,7 +654,7 @@ impl ClientState { } self.forwarded_dns_queries - .insert(id, (original_src, now + IDS_EXPIRE)); + .insert((id, server), (original_src, now + IDS_EXPIRE)); self.buffered_transmits.push_back(Transmit { src: None, dst: server, @@ -677,14 +682,15 @@ impl ClientState { let message = Message::from_slice(packet).ok()?; let query_id = message.header().id(); - let (destination, _) = self.forwarded_dns_queries.remove(&query_id)?; + let (destination, _) = self.forwarded_dns_queries.remove(&(query_id, from))?; let daddr = destination.ip(); let dport = destination.port(); - Some( - ip_packet::make::udp_packet(saddr, daddr, sport, dport, packet.to_vec()) - .into_immutable(), - ) + let ip_packet = ip_packet::make::udp_packet(saddr, daddr, sport, dport, packet.to_vec()) + .inspect_err(|_| tracing::warn!("Failed to find original dst for DNS response")) + .ok()?; + + Some(ip_packet.into_immutable()) } pub fn on_connection_failed(&mut self, resource: ResourceId) { diff --git a/rust/connlib/tunnel/src/dns.rs b/rust/connlib/tunnel/src/dns.rs index 5c47056e3..2bc535267 100644 --- a/rust/connlib/tunnel/src/dns.rs +++ b/rust/connlib/tunnel/src/dns.rs @@ -228,6 +228,7 @@ impl StubResolver { datagram.get_source(), response, ) + .expect("src and dst come from the same packet") .into_immutable(); return Some(ResolveStrategy::LocalResponse(packet)); @@ -271,6 +272,7 @@ impl StubResolver { datagram.get_source(), response, ) + .expect("src and dst come from the same packet") .into_immutable(); Some(ResolveStrategy::LocalResponse(packet)) diff --git a/rust/connlib/tunnel/src/peer.rs b/rust/connlib/tunnel/src/peer.rs index f400bde8f..b687bf07d 100644 --- a/rust/connlib/tunnel/src/peer.rs +++ b/rust/connlib/tunnel/src/peer.rs @@ -652,7 +652,8 @@ mod tests { 5401, 80, vec![0; 100], - ); + ) + .unwrap(); let udp_packet = ip_packet::make::udp_packet( source_v4_addr(), @@ -660,7 +661,8 @@ mod tests { 5401, 80, vec![0; 100], - ); + ) + .unwrap(); peer.expire_resources(now); @@ -1041,7 +1043,8 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, *dport, payload.clone()), Protocol::Udp { dport } => udp_packet(src, dest, sport, *dport, payload.clone()), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); assert!(peer.ensure_allowed_dst(&packet).is_ok()); } } @@ -1073,7 +1076,8 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload.clone()), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload.clone()), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); assert!(peer.ensure_allowed_dst(&packet).is_ok()); } } @@ -1114,7 +1118,8 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload.clone()), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload.clone()), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); assert!(peer.ensure_allowed_dst(&packet).is_ok()); } @@ -1128,7 +1133,8 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload.clone()), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload.clone()), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); assert!(peer.ensure_allowed_dst(&packet).is_ok()); } } @@ -1170,7 +1176,8 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload.clone()), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload.clone()), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); assert!(peer.ensure_allowed_dst(&packet).is_ok()); } @@ -1200,7 +1207,8 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); peer.add_resource(vec![resource_addr], resource_id, filters, None, None); @@ -1240,13 +1248,15 @@ mod proptests { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload.clone()), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload.clone()), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); let packet_rejected = match protocol_removed { Protocol::Tcp { dport } => tcp_packet(src, dest, sport, dport, payload), Protocol::Udp { dport } => udp_packet(src, dest, sport, dport, payload), Protocol::Icmp => icmp_request_packet(src, dest, 1, 0), - }; + } + .unwrap(); peer.add_resource( vec![supernet(resource_addr).unwrap_or(resource_addr)], diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index 46ff466fe..61cf057aa 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -101,7 +101,8 @@ impl SimClient { SocketAddr::new(src, 9999), // An application would pick a random source port that is free. SocketAddr::new(dns_server, 53), query_id, - ); + ) + .unwrap(); self.encapsulate(packet, now) } diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index fc5ce336c..53f5df1c1 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -180,7 +180,8 @@ impl StateMachineTest for TunnelTest { identifier, .. } => { - let packet = ip_packet::make::icmp_request_packet(src, dst, seq, identifier); + let packet = + ip_packet::make::icmp_request_packet(src, dst, seq, identifier).unwrap(); let transmit = state.client.exec_mut(|sim| sim.encapsulate(packet, now)); @@ -207,7 +208,8 @@ impl StateMachineTest for TunnelTest { }); let dst = *resolved_ip.select(available_ips); - let packet = ip_packet::make::icmp_request_packet(src, dst, seq, identifier); + let packet = + ip_packet::make::icmp_request_packet(src, dst, seq, identifier).unwrap(); let transmit = state .client diff --git a/rust/ip-packet/src/make.rs b/rust/ip-packet/src/make.rs index 24d27b7a0..c2e6e5e92 100644 --- a/rust/ip-packet/src/make.rs +++ b/rust/ip-packet/src/make.rs @@ -22,7 +22,7 @@ pub fn icmp_request_packet( dst: impl Into, seq: u16, identifier: u16, -) -> MutableIpPacket<'static> { +) -> Result, IpVersionMismatch> { icmp_packet(src, dst.into(), seq, identifier, IcmpKind::Request) } @@ -31,7 +31,7 @@ pub fn icmp_reply_packet( dst: impl Into, seq: u16, identifier: u16, -) -> MutableIpPacket<'static> { +) -> Result, IpVersionMismatch> { icmp_packet(src, dst.into(), seq, identifier, IcmpKind::Response) } @@ -48,6 +48,7 @@ pub fn icmp_response_packet(packet: IpPacket<'static>) -> MutableIpPacket<'stati echo_request.identifier(), IcmpKind::Response, ) + .expect("src and dst come from the same packet") } #[cfg_attr(test, derive(Debug, test_strategy::Arbitrary))] @@ -115,11 +116,11 @@ pub(crate) fn icmp_packet( seq: u16, identifier: u16, kind: IcmpKind, -) -> MutableIpPacket<'static> { +) -> Result, IpVersionMismatch> { match (src, dst) { - (IpAddr::V4(src), IpAddr::V4(dst)) => { - icmp4_packet_with_options(src, dst, seq, identifier, kind, 5) - } + (IpAddr::V4(src), IpAddr::V4(dst)) => Ok(icmp4_packet_with_options( + src, dst, seq, identifier, kind, 5, + )), (IpAddr::V6(src), IpAddr::V6(dst)) => { use crate::{ icmpv6::{ @@ -155,11 +156,10 @@ pub(crate) fn icmp_packet( let mut result = MutableIpPacket::owned(buf).unwrap(); result.update_checksum(); - result - } - (IpAddr::V6(_), IpAddr::V4(_)) | (IpAddr::V4(_), IpAddr::V6(_)) => { - panic!("IPs must be of the same version") + + Ok(result) } + (IpAddr::V6(_), IpAddr::V4(_)) | (IpAddr::V4(_), IpAddr::V6(_)) => Err(IpVersionMismatch), } } @@ -169,7 +169,7 @@ pub fn tcp_packet( sport: u16, dport: u16, payload: Vec, -) -> MutableIpPacket<'static> +) -> Result, IpVersionMismatch> where IP: Into, { @@ -187,7 +187,7 @@ where ipv4_header(src, dst, IpNextHeaderProtocols::Tcp, 5, &mut buf[20..]); tcp_header(saddr, daddr, sport, dport, &payload, &mut buf[40..]); - MutableIpPacket::owned(buf).unwrap() + Ok(MutableIpPacket::owned(buf).unwrap()) } (IpAddr::V6(src), IpAddr::V6(dst)) => { use crate::ip::IpNextHeaderProtocols; @@ -197,11 +197,9 @@ where ipv6_header(src, dst, IpNextHeaderProtocols::Tcp, &mut buf[20..]); tcp_header(saddr, daddr, sport, dport, &payload, &mut buf[60..]); - MutableIpPacket::owned(buf).unwrap() - } - (IpAddr::V6(_), IpAddr::V4(_)) | (IpAddr::V4(_), IpAddr::V6(_)) => { - panic!("IPs must be of the same version") + Ok(MutableIpPacket::owned(buf).unwrap()) } + (IpAddr::V6(_), IpAddr::V4(_)) | (IpAddr::V4(_), IpAddr::V6(_)) => Err(IpVersionMismatch), } } @@ -211,7 +209,7 @@ pub fn udp_packet( sport: u16, dport: u16, payload: Vec, -) -> MutableIpPacket<'static> +) -> Result, IpVersionMismatch> where IP: Into, { @@ -228,7 +226,7 @@ where ipv4_header(src, dst, IpNextHeaderProtocols::Udp, 5, &mut buf[20..]); udp_header(saddr, daddr, sport, dport, &payload, &mut buf[40..]); - MutableIpPacket::owned(buf).unwrap() + Ok(MutableIpPacket::owned(buf).unwrap()) } (IpAddr::V6(src), IpAddr::V6(dst)) => { use crate::ip::IpNextHeaderProtocols; @@ -238,11 +236,9 @@ where ipv6_header(src, dst, IpNextHeaderProtocols::Udp, &mut buf[20..]); udp_header(saddr, daddr, sport, dport, &payload, &mut buf[60..]); - MutableIpPacket::owned(buf).unwrap() - } - (IpAddr::V6(_), IpAddr::V4(_)) | (IpAddr::V4(_), IpAddr::V6(_)) => { - panic!("IPs must be of the same version") + Ok(MutableIpPacket::owned(buf).unwrap()) } + (IpAddr::V6(_), IpAddr::V4(_)) | (IpAddr::V4(_), IpAddr::V6(_)) => Err(IpVersionMismatch), } } @@ -252,7 +248,7 @@ pub fn dns_query( src: SocketAddr, dst: SocketAddr, id: u16, -) -> MutableIpPacket<'static> { +) -> Result, IpVersionMismatch> { // Create the DNS query message let mut msg_builder = MessageBuilder::new_vec(); @@ -318,8 +314,13 @@ where udp.get_source(), payload, ) + .expect("src and dst are retrieved from the same packet") } +#[derive(thiserror::Error, Debug)] +#[error("IPs must be of the same version")] +pub struct IpVersionMismatch; + fn ipv4_header( src: Ipv4Addr, dst: Ipv4Addr, diff --git a/rust/ip-packet/src/proptest.rs b/rust/ip-packet/src/proptest.rs index b224369b6..ba34576d0 100644 --- a/rust/ip-packet/src/proptest.rs +++ b/rust/ip-packet/src/proptest.rs @@ -5,10 +5,10 @@ use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; pub fn udp_packet() -> impl Strategy> { prop_oneof![ (ip4_tuple(), any::(), any::()).prop_map(|((saddr, daddr), sport, dport)| { - crate::make::udp_packet(saddr, daddr, sport, dport, Vec::new()) + crate::make::udp_packet(saddr, daddr, sport, dport, Vec::new()).unwrap() }), (ip6_tuple(), any::(), any::()).prop_map(|((saddr, daddr), sport, dport)| { - crate::make::udp_packet(saddr, daddr, sport, dport, Vec::new()) + crate::make::udp_packet(saddr, daddr, sport, dport, Vec::new()).unwrap() }), ] } @@ -16,10 +16,10 @@ pub fn udp_packet() -> impl Strategy> { pub fn tcp_packet() -> impl Strategy> { prop_oneof![ (ip4_tuple(), any::(), any::()).prop_map(|((saddr, daddr), sport, dport)| { - crate::make::tcp_packet(saddr, daddr, sport, dport, Vec::new()) + crate::make::tcp_packet(saddr, daddr, sport, dport, Vec::new()).unwrap() }), (ip6_tuple(), any::(), any::()).prop_map(|((saddr, daddr), sport, dport)| { - crate::make::tcp_packet(saddr, daddr, sport, dport, Vec::new()) + crate::make::tcp_packet(saddr, daddr, sport, dport, Vec::new()).unwrap() }), ] } @@ -27,10 +27,10 @@ pub fn tcp_packet() -> impl Strategy> { pub fn icmp_request_packet() -> impl Strategy> { prop_oneof![ (ip4_tuple(), any::(), any::()).prop_map(|((saddr, daddr), sport, dport)| { - crate::make::icmp_request_packet(IpAddr::V4(saddr), daddr, sport, dport) + crate::make::icmp_request_packet(IpAddr::V4(saddr), daddr, sport, dport).unwrap() }), (ip6_tuple(), any::(), any::()).prop_map(|((saddr, daddr), sport, dport)| { - crate::make::icmp_request_packet(IpAddr::V6(saddr), daddr, sport, dport) + crate::make::icmp_request_packet(IpAddr::V6(saddr), daddr, sport, dport).unwrap() }), ] } diff --git a/rust/ip-packet/src/proptests.rs b/rust/ip-packet/src/proptests.rs index 58b789060..37c36b122 100644 --- a/rust/ip-packet/src/proptests.rs +++ b/rust/ip-packet/src/proptests.rs @@ -16,7 +16,9 @@ fn tcp_packet_v4() -> impl Strategy> { any::(), any::>(), ) - .prop_map(|(src, dst, sport, dport, payload)| tcp_packet(src, dst, sport, dport, payload)) + .prop_map(|(src, dst, sport, dport, payload)| { + tcp_packet(src, dst, sport, dport, payload).unwrap() + }) } fn tcp_packet_v6() -> impl Strategy> { @@ -27,7 +29,9 @@ fn tcp_packet_v6() -> impl Strategy> { any::(), any::>(), ) - .prop_map(|(src, dst, sport, dport, payload)| tcp_packet(src, dst, sport, dport, payload)) + .prop_map(|(src, dst, sport, dport, payload)| { + tcp_packet(src, dst, sport, dport, payload).unwrap() + }) } fn udp_packet_v4() -> impl Strategy> { @@ -38,7 +42,9 @@ fn udp_packet_v4() -> impl Strategy> { any::(), any::>(), ) - .prop_map(|(src, dst, sport, dport, payload)| udp_packet(src, dst, sport, dport, payload)) + .prop_map(|(src, dst, sport, dport, payload)| { + udp_packet(src, dst, sport, dport, payload).unwrap() + }) } fn udp_packet_v6() -> impl Strategy> { @@ -49,7 +55,9 @@ fn udp_packet_v6() -> impl Strategy> { any::(), any::>(), ) - .prop_map(|(src, dst, sport, dport, payload)| udp_packet(src, dst, sport, dport, payload)) + .prop_map(|(src, dst, sport, dport, payload)| { + udp_packet(src, dst, sport, dport, payload).unwrap() + }) } fn icmp_packet_v4() -> impl Strategy> { @@ -60,7 +68,9 @@ fn icmp_packet_v4() -> impl Strategy> { any::(), any::(), ) - .prop_map(|(src, dst, id, seq, kind)| icmp_packet(src.into(), dst.into(), id, seq, kind)) + .prop_map(|(src, dst, id, seq, kind)| { + icmp_packet(src.into(), dst.into(), id, seq, kind).unwrap() + }) } fn icmp_packet_v4_header_options() -> impl Strategy> { @@ -85,7 +95,9 @@ fn icmp_packet_v6() -> impl Strategy> { any::(), any::(), ) - .prop_map(|(src, dst, id, seq, kind)| icmp_packet(src.into(), dst.into(), id, seq, kind)) + .prop_map(|(src, dst, id, seq, kind)| { + icmp_packet(src.into(), dst.into(), id, seq, kind).unwrap() + }) } fn packet() -> impl Strategy> { diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index c89015b42..195e23e76 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -19,6 +19,13 @@ export default function GUI({ title }: { title: string }) { */} + +
    + + Fixes an issue where the IPC service can panic during DNS resolution. + +
+