github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 18:18:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(connlib): index forwarded DNS queries by ID + socket (#6233)

Browse Source 
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.
This commit is contained in:
Thomas Eizinger
2024-08-09 08:01:57 +01:00
committed by GitHub
parent 67ae8ff380
commit 4ae64f0257
10 changed files with 98 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
rust/bin-shared/benches/tunnel.rs
View File

@@ -91,6 +91,7 @@ mod platform {
original_udp.get_source(),
vec![RESP_CODE],
)
.unwrap()
})
.packet();
tun.write4(res_buf)?;

22
rust/connlib/tunnel/src/client.rs
View File

@@ -261,13 +261,18 @@ pub struct ClientState {
///
/// The [`Instant`] tracks when the DNS query expires.
mangled_dns_queries: HashMap<u16, Instant>,
/// 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<u16, (SocketAddr, Instant)>,
///
/// 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) {

2
rust/connlib/tunnel/src/dns.rs
View File

@@ -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))

30
rust/connlib/tunnel/src/peer.rs
View File

@@ -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)],

3
rust/connlib/tunnel/src/tests/sim_client.rs
View File

@@ -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)
}

6
rust/connlib/tunnel/src/tests/sut.rs
View File

@@ -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

47
rust/ip-packet/src/make.rs
View File

@@ -22,7 +22,7 @@ pub fn icmp_request_packet(
dst: impl Into<IpAddr>,
seq: u16,
identifier: u16,
) -> MutableIpPacket<'static> {
) -> Result<MutableIpPacket<'static>, IpVersionMismatch> {
icmp_packet(src, dst.into(), seq, identifier, IcmpKind::Request)
}
@@ -31,7 +31,7 @@ pub fn icmp_reply_packet(
dst: impl Into<IpAddr>,
seq: u16,
identifier: u16,
) -> MutableIpPacket<'static> {
) -> Result<MutableIpPacket<'static>, 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<MutableIpPacket<'static>, 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<IP>(
sport: u16,
dport: u16,
payload: Vec<u8>,
) -> MutableIpPacket<'static>
) -> Result<MutableIpPacket<'static>, IpVersionMismatch>
where
IP: Into<IpAddr>,
{
@@ -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<IP>(
sport: u16,
dport: u16,
payload: Vec<u8>,
) -> MutableIpPacket<'static>
) -> Result<MutableIpPacket<'static>, IpVersionMismatch>
where
IP: Into<IpAddr>,
{
@@ -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<MutableIpPacket<'static>, 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,

12
rust/ip-packet/src/proptest.rs
View File

@@ -5,10 +5,10 @@ use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
pub fn udp_packet() -> impl Strategy<Value = MutableIpPacket<'static>> {
prop_oneof![
(ip4_tuple(), any::<u16>(), any::<u16>()).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::<u16>(), any::<u16>()).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<Value = MutableIpPacket<'static>> {
pub fn tcp_packet() -> impl Strategy<Value = MutableIpPacket<'static>> {
prop_oneof![
(ip4_tuple(), any::<u16>(), any::<u16>()).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::<u16>(), any::<u16>()).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<Value = MutableIpPacket<'static>> {
pub fn icmp_request_packet() -> impl Strategy<Value = MutableIpPacket<'static>> {
prop_oneof![
(ip4_tuple(), any::<u16>(), any::<u16>()).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::<u16>(), any::<u16>()).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()
}),
]
}

24
rust/ip-packet/src/proptests.rs
View File

@@ -16,7 +16,9 @@ fn tcp_packet_v4() -> impl Strategy<Value = MutableIpPacket<'static>> {
any::<u16>(),
any::<Vec<u8>>(),
)
.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<Value = MutableIpPacket<'static>> {
@@ -27,7 +29,9 @@ fn tcp_packet_v6() -> impl Strategy<Value = MutableIpPacket<'static>> {
any::<u16>(),
any::<Vec<u8>>(),
)
.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<Value = MutableIpPacket<'static>> {
@@ -38,7 +42,9 @@ fn udp_packet_v4() -> impl Strategy<Value = MutableIpPacket<'static>> {
any::<u16>(),
any::<Vec<u8>>(),
)
.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<Value = MutableIpPacket<'static>> {
@@ -49,7 +55,9 @@ fn udp_packet_v6() -> impl Strategy<Value = MutableIpPacket<'static>> {
any::<u16>(),
any::<Vec<u8>>(),
)
.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<Value = MutableIpPacket<'static>> {
@@ -60,7 +68,9 @@ fn icmp_packet_v4() -> impl Strategy<Value = MutableIpPacket<'static>> {
any::<u16>(),
any::<IcmpKind>(),
)
.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<Value = MutableIpPacket<'static>> {
@@ -85,7 +95,9 @@ fn icmp_packet_v6() -> impl Strategy<Value = MutableIpPacket<'static>> {
any::<u16>(),
any::<IcmpKind>(),
)
.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<Value = MutableIpPacket<'static>> {

7
website/src/components/Changelog/GUI.tsx
View File

@@ -19,6 +19,13 @@ export default function GUI({ title }: { title: string }) {
</ul>
</Entry>
*/}
<Entry version="1.1.11" date={new Date("2024-08-09")}>
<ul className="list-disc space-y-2 pl-4 mb-4">
<ChangeItem pull="6233">
Fixes an issue where the IPC service can panic during DNS resolution.
</ChangeItem>
</ul>
</Entry>
<Entry version="1.1.10" date={new Date("2024-08-08")}>
<ul className="list-disc space-y-2 pl-4 mb-4">
<ChangeItem pull="5923">