From 33d5c32f35681c8e0b94a38579dff1238514b69f Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 9 May 2025 11:08:31 +0930 Subject: [PATCH] fix(gateway): truncate payload of ICMP errors (#9059) When the Gateway is handed an IP packet for a DNS resource that it cannot route, it sends back an ICMP unreachable error. According to RFC 792 [0] (for ICMPv4) and RFC 4443 [1] (for ICMPv6), parts of the original packet should be included in the ICMP error payload to allow the sending party to correlate, what could not be sent. For ICMPv4, the RFC says: ``` Internet Header + 64 bits of Data Datagram The internet header plus the first 64 bits of the original datagram's data. This data is used by the host to match the message to the appropriate process. If a higher level protocol uses port numbers, they are assumed to be in the first 64 data bits of the original datagram's data. ``` For ICMPv6, the RFC says: ``` As much of invoking packet as possible without the ICMPv6 packet exceeding the minimum IPv6 MTU ``` [0]: https://datatracker.ietf.org/doc/html/rfc792 [1]: https://datatracker.ietf.org/doc/html/rfc4443#section-3.1 --- rust/connlib/tunnel/src/peer.rs | 61 +++------- rust/ip-packet/src/make.rs | 121 ++++++++++++++++++- website/src/components/Changelog/Gateway.tsx | 7 +- 3 files changed, 140 insertions(+), 49 deletions(-) diff --git a/rust/connlib/tunnel/src/peer.rs b/rust/connlib/tunnel/src/peer.rs index 7ef595314..d13f82080 100644 --- a/rust/connlib/tunnel/src/peer.rs +++ b/rust/connlib/tunnel/src/peer.rs @@ -1,6 +1,6 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, VecDeque, hash_map}; use std::iter; -use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; +use std::net::{IpAddr, SocketAddr}; use std::time::Instant; use crate::client::{IPV4_RESOURCES, IPV6_RESOURCES}; @@ -12,7 +12,7 @@ use dns_types::DomainName; use filter_engine::FilterEngine; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use ip_network_table::IpNetworkTable; -use ip_packet::{IpPacket, PacketBuilder, Protocol, UnsupportedProtocol, icmpv4, icmpv6}; +use ip_packet::{IpPacket, Protocol, UnsupportedProtocol}; use crate::utils::network_contains_network; use crate::{GatewayEvent, IpConfig, otel}; @@ -294,13 +294,25 @@ impl ClientOnGateway { } let Some(state) = self.permanent_translations.get_mut(&packet.destination()) else { - return self.dst_unreachable(&packet); + return Ok(TranslateOutboundResult::DestinationUnreachable( + ip_packet::make::icmp_dst_unreachable( + self.gateway_tun.v4, + self.gateway_tun.v6, + &packet, + )?, + )); }; #[expect(clippy::collapsible_if, reason = "We want the feature flag separate.")] if firezone_telemetry::feature_flags::icmp_unreachable_instead_of_nat64() { if state.resolved_ip.is_ipv4() != dst.is_ipv4() { - return self.dst_unreachable(&packet); + return Ok(TranslateOutboundResult::DestinationUnreachable( + ip_packet::make::icmp_dst_unreachable( + self.gateway_tun.v4, + self.gateway_tun.v6, + &packet, + )?, + )); } } @@ -473,47 +485,6 @@ impl ClientOnGateway { pub fn id(&self) -> ClientId { self.id } - - fn dst_unreachable(&self, packet: &IpPacket) -> Result { - let src = packet.source(); - - let icmp_error = match src { - IpAddr::V4(src) => icmpv4_network_unreachable(self.gateway_tun.v4, src, packet)?, - IpAddr::V6(src) => icmpv6_address_unreachable(self.gateway_tun.v6, src, packet)?, - }; - - Ok(TranslateOutboundResult::DestinationUnreachable(icmp_error)) - } -} - -fn icmpv4_network_unreachable( - src: Ipv4Addr, - dst: Ipv4Addr, - original_packet: &IpPacket, -) -> Result { - let builder = PacketBuilder::ipv4(src.octets(), dst.octets(), 20).icmpv4( - ip_packet::Icmpv4Type::DestinationUnreachable(icmpv4::DestUnreachableHeader::Network), - ); - let payload = original_packet.packet(); - - let ip_packet = ip_packet::build!(builder, payload)?; - - Ok(ip_packet) -} - -fn icmpv6_address_unreachable( - src: Ipv6Addr, - dst: Ipv6Addr, - original_packet: &IpPacket, -) -> Result { - let builder = PacketBuilder::ipv6(src.octets(), dst.octets(), 20).icmpv6( - ip_packet::Icmpv6Type::DestinationUnreachable(icmpv6::DestUnreachableCode::Address), - ); - let payload = original_packet.packet(); - - let ip_packet = ip_packet::build!(builder, payload)?; - - Ok(ip_packet) } #[derive(Debug, PartialEq)] diff --git a/rust/ip-packet/src/make.rs b/rust/ip-packet/src/make.rs index 57f61f190..54af34ccf 100644 --- a/rust/ip-packet/src/make.rs +++ b/rust/ip-packet/src/make.rs @@ -1,9 +1,9 @@ //! Factory module for making all kinds of packets. -use crate::{IpPacket, IpPacketBuf}; +use crate::{IpPacket, IpPacketBuf, MAX_IP_SIZE}; use anyhow::{Context as _, Result, bail}; -use etherparse::PacketBuilder; -use std::net::IpAddr; +use etherparse::{Icmpv6Header, Ipv6Header, PacketBuilder, icmpv4, icmpv6}; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; /// Helper macro to turn a [`PacketBuilder`] into an [`IpPacket`]. #[macro_export] @@ -156,6 +156,121 @@ where } } +pub fn icmp_dst_unreachable( + ipv4_src: Ipv4Addr, + ipv6_src: Ipv6Addr, + original_packet: &IpPacket, +) -> Result { + let src = original_packet.source(); + + let icmp_error = match src { + IpAddr::V4(src) => icmpv4_network_unreachable(ipv4_src, src, original_packet)?, + IpAddr::V6(src) => icmpv6_address_unreachable(ipv6_src, src, original_packet)?, + }; + + Ok(icmp_error) +} + +fn icmpv4_network_unreachable( + src: Ipv4Addr, + dst: Ipv4Addr, + original_packet: &IpPacket, +) -> Result { + let builder = PacketBuilder::ipv4(src.octets(), dst.octets(), 20).icmpv4( + crate::Icmpv4Type::DestinationUnreachable(icmpv4::DestUnreachableHeader::Network), + ); + let payload = original_packet.packet(); + + let header_len = original_packet + .ipv4_header() + .context("Not an IPv4 packet")? + .header_len(); + let icmp_error_payload_len = header_len + 8; + + let actual_payload_len = std::cmp::min(payload.len(), icmp_error_payload_len); + let error_payload = &payload[..actual_payload_len]; + + let ip_packet = crate::build!(builder, error_payload)?; + + Ok(ip_packet) +} + +fn icmpv6_address_unreachable( + src: Ipv6Addr, + dst: Ipv6Addr, + original_packet: &IpPacket, +) -> Result { + const MAX_ICMP_ERROR_PAYLOAD_LEN: usize = MAX_IP_SIZE - Ipv6Header::LEN - Icmpv6Header::MAX_LEN; + + let builder = PacketBuilder::ipv6(src.octets(), dst.octets(), 20).icmpv6( + crate::Icmpv6Type::DestinationUnreachable(icmpv6::DestUnreachableCode::Address), + ); + let payload = original_packet.packet(); + + let actual_payload_len = std::cmp::min(payload.len(), MAX_ICMP_ERROR_PAYLOAD_LEN); + let error_payload = &payload[..actual_payload_len]; + + let ip_packet = crate::build!(builder, error_payload)?; + + Ok(ip_packet) +} + #[derive(thiserror::Error, Debug)] #[error("IPs must be of the same version")] pub struct IpVersionMismatch; + +#[cfg(all(test, feature = "proptest"))] +mod tests { + use etherparse::{Ipv4Header, Ipv6Header, UdpHeader}; + use proptest::{ + collection, + prelude::{Strategy, any}, + }; + + use crate::MAX_IP_SIZE; + + use super::*; + + #[test_strategy::proptest()] + fn ipv4_icmp_unreachable( + #[strategy(payload(MAX_IP_SIZE - Ipv4Header::MIN_LEN - UdpHeader::LEN))] payload: Vec, + ) { + let unreachable_packet = + udp_packet(Ipv4Addr::LOCALHOST, Ipv4Addr::LOCALHOST, 0, 0, payload).unwrap(); + + let icmp_error = + icmp_dst_unreachable(ERROR_SRC_IPV4, ERROR_SRC_IPV6, &unreachable_packet).unwrap(); + + assert_eq!(icmp_error.destination(), IpAddr::V4(Ipv4Addr::LOCALHOST)); + assert_eq!(icmp_error.source(), IpAddr::V4(ERROR_SRC_IPV4)); + assert!(matches!( + icmp_error.icmp_unreachable_destination(), + Ok(Some(_)) + )); + } + + #[test_strategy::proptest()] + fn ipv6_icmp_unreachable_max_payload( + #[strategy(payload(MAX_IP_SIZE - Ipv6Header::LEN - UdpHeader::LEN))] payload: Vec, + ) { + let unreachable_packet = + udp_packet(Ipv6Addr::LOCALHOST, Ipv6Addr::LOCALHOST, 0, 0, payload).unwrap(); + + let icmp_error = + icmp_dst_unreachable(ERROR_SRC_IPV4, ERROR_SRC_IPV6, &unreachable_packet).unwrap(); + + assert_eq!(icmp_error.destination(), IpAddr::V6(Ipv6Addr::LOCALHOST)); + assert_eq!(icmp_error.source(), IpAddr::V6(ERROR_SRC_IPV6)); + assert!(matches!( + icmp_error.icmp_unreachable_destination(), + Ok(Some(_)) + )); + } + + const ERROR_SRC_IPV4: Ipv4Addr = Ipv4Addr::new(1, 1, 1, 1); + const ERROR_SRC_IPV6: Ipv6Addr = Ipv6Addr::new(1, 1, 1, 1, 1, 1, 1, 1); + + fn payload(max_size: usize) -> impl Strategy> { + collection::vec(any::(), 0..=max_size) + } +} diff --git a/website/src/components/Changelog/Gateway.tsx b/website/src/components/Changelog/Gateway.tsx index 117ceacf6..26763aa6e 100644 --- a/website/src/components/Changelog/Gateway.tsx +++ b/website/src/components/Changelog/Gateway.tsx @@ -22,7 +22,12 @@ export default function Gateway() { return ( - + + + Fixes an issue where ICMP unreachable errors for large packets would + not be sent. + + Fixes an issue where ECN bits got erroneously cleared without updating