From 4ae64f0257d19be72d17fe395f12f79303e668ae Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 9 Aug 2024 08:01:57 +0100 Subject: [PATCH] fix(connlib): index forwarded DNS queries by ID + socket (#6233) When forwarding DNS queries, we need to remember the original source socket in order to send the response back. Previously, this mapping was indexed by the DNS query ID. As it turns out, at least Windows doesn't have a global DNS query ID counter and may reuse them across different DNS servers. If that happens and two of these queries overlap, then we match the wrong responses together. In the best case, this produces bad DNS results on the client. In the worst case, those queries were for DNS servers with different IP versions in which case we triggered a panic in connlib further down the stack where we created the IP packet for the response. To fix this, we first and foremost remove the explicit `panic!` from the `make::` functions in `ip-packet`. Originally, these functions were only used in tests but we started to use them in production code too and unfortunately forgot about this panic. By introducing a `Result`, all call-sites are made aware that this can fail. Second, we fix the actual indexing into the data structure for forwarded DNS queries to also include the DNS server's socket. This ensures we don't treat the DNS query IDs as globally unique. Third, we replace the panicking path in `try_handle_forwarded_dns_response` with a log statement, meaning if the above assumption turns out wrong for some reason, we still don't panic and simply don't handle the packet. --- rust/bin-shared/benches/tunnel.rs | 1 + rust/connlib/tunnel/src/client.rs | 22 ++++++---- rust/connlib/tunnel/src/dns.rs | 2 + rust/connlib/tunnel/src/peer.rs | 30 ++++++++----- rust/connlib/tunnel/src/tests/sim_client.rs | 3 +- rust/connlib/tunnel/src/tests/sut.rs | 6 ++- rust/ip-packet/src/make.rs | 47 +++++++++++---------- rust/ip-packet/src/proptest.rs | 12 +++--- rust/ip-packet/src/proptests.rs | 24 ++++++++--- website/src/components/Changelog/GUI.tsx | 7 +++ 10 files changed, 98 insertions(+), 56 deletions(-) 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. + +
+