From bc2febed996a53bc7bda1daef43beeb5adfb6f72 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 19 Dec 2024 18:15:43 +0100 Subject: [PATCH] fix(connlib): use correct constant for truncating DNS responses (#7551) In case an upstream DNS server responds with a payload that exceeds the available buffer space of an IP packet, we need to truncate the response. Currently, this truncation uses the **wrong** constant to check for the maximum allowed length. Instead of the `MAX_DATAGRAM_PAYLOAD`, we actually need to check against a limit that is less than the MTU as the IP layer and the UDP layer both add an overhead. To fix this, we introduce such a constant and provide additional documentation on the remaining ones to hopefully avoid future errors. --- rust/connlib/snownet/src/node.rs | 4 +- rust/connlib/tunnel/src/client.rs | 43 +++++++++---------- rust/connlib/tunnel/src/io.rs | 6 +-- rust/connlib/tunnel/src/p2p_control.rs | 2 +- .../tunnel/src/tests/dns_server_resource.rs | 6 +-- rust/dns-over-tcp/src/stub_device.rs | 4 +- rust/gateway/src/main.rs | 2 +- rust/headless-client/src/ipc_service.rs | 2 +- rust/headless-client/src/main.rs | 2 +- rust/ip-packet/src/buffer_pool.rs | 6 +-- rust/ip-packet/src/lib.rs | 30 ++++++++++--- rust/ip-packet/src/make.rs | 2 +- 12 files changed, 63 insertions(+), 46 deletions(-) diff --git a/rust/connlib/snownet/src/node.rs b/rust/connlib/snownet/src/node.rs index 6fe0d0ac0..974271e49 100644 --- a/rust/connlib/snownet/src/node.rs +++ b/rust/connlib/snownet/src/node.rs @@ -182,7 +182,7 @@ where connections: Default::default(), stats: Default::default(), buffer_pool: Arc::new(lockfree_object_pool::SpinLockObjectPool::new( - || vec![0; ip_packet::MAX_DATAGRAM_PAYLOAD], + || vec![0; ip_packet::MAX_FZ_PAYLOAD], |v| v.fill(0), )), } @@ -735,7 +735,7 @@ where wg_timer: DEFAULT_WG_TIMER, next_wg_timer_update: now, stats: Default::default(), - buffer: vec![0; ip_packet::MAX_DATAGRAM_PAYLOAD], + buffer: vec![0; ip_packet::MAX_FZ_PAYLOAD], intent_sent_at, signalling_completed_at: now, remote_pub_key: remote, diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 13e815414..a67eead0b 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -21,7 +21,7 @@ use firezone_logging::{ }; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use ip_network_table::IpNetworkTable; -use ip_packet::{IpPacket, UdpSlice, MAX_DATAGRAM_PAYLOAD}; +use ip_packet::{IpPacket, UdpSlice, MAX_UDP_PAYLOAD}; use itertools::Itertools; use crate::peer::GatewayOnClient; @@ -576,7 +576,7 @@ impl ClientState { }); unwrap_or_warn!( - self.try_queue_udp_dns_response(server, source, &message), + self.try_queue_udp_dns_response(server, source, message), "Failed to queue UDP DNS response" ); } @@ -647,7 +647,7 @@ impl ClientState { &mut self, from: SocketAddr, dst: SocketAddr, - message: &Message>, + message: Message>, ) -> anyhow::Result<()> { let saddr = *self .dns_mapping @@ -1139,7 +1139,7 @@ impl ClientState { self.update_dns_resource_nat(now, iter::empty()); unwrap_or_debug!( - self.try_queue_udp_dns_response(upstream, source, &response), + self.try_queue_udp_dns_response(upstream, source, response), "Failed to queue UDP DNS response: {}" ); } @@ -1826,26 +1826,25 @@ fn maybe_mangle_dns_response_from_cidr_resource( packet } -fn truncate_dns_response(message: &Message>) -> Vec { - let mut message_bytes = message.as_octets().to_vec(); - - if message_bytes.len() > MAX_DATAGRAM_PAYLOAD { - tracing::debug!(?message, message_length = %message_bytes.len(), "Too big DNS response, truncating"); - - let mut new_message = message.clone(); - new_message.header_mut().set_tc(true); - - let message_truncation = match message.answer() { - Ok(answer) if answer.pos() <= MAX_DATAGRAM_PAYLOAD => answer.pos(), - // This should be very unlikely or impossible. - _ => message.question().pos(), - }; - - message_bytes = new_message.as_octets().to_vec(); - - message_bytes.truncate(message_truncation); +fn truncate_dns_response(mut message: Message>) -> Vec { + let message_length = message.as_octets().len(); + if message_length <= MAX_UDP_PAYLOAD { + return message.into_octets(); } + tracing::debug!(?message, %message_length, "Too big DNS response, truncating"); + + message.header_mut().set_tc(true); + + let message_truncation = match message.answer() { + Ok(answer) if answer.pos() <= MAX_UDP_PAYLOAD => answer.pos(), + // This should be very unlikely or impossible. + _ => message.question().pos(), + }; + + let mut message_bytes = message.into_octets(); + message_bytes.truncate(message_truncation); + message_bytes } diff --git a/rust/connlib/tunnel/src/io.rs b/rust/connlib/tunnel/src/io.rs index af9bc5b24..cc22d70c4 100644 --- a/rust/connlib/tunnel/src/io.rs +++ b/rust/connlib/tunnel/src/io.rs @@ -6,7 +6,7 @@ use firezone_logging::{telemetry_event, telemetry_span}; use futures_bounded::FuturesTupleSet; use futures_util::FutureExt as _; use gso_queue::GsoQueue; -use ip_packet::{IpPacket, MAX_DATAGRAM_PAYLOAD}; +use ip_packet::{IpPacket, MAX_FZ_PAYLOAD}; use socket_factory::{DatagramIn, SocketFactory, TcpSocket, UdpSocket}; use std::{ collections::VecDeque, @@ -316,8 +316,8 @@ impl Io { fn is_max_wg_packet_size(d: &DatagramIn) -> bool { let len = d.packet.len(); - if len > MAX_DATAGRAM_PAYLOAD { - telemetry_event!(from = %d.from, %len, "Dropping too large datagram (max allowed: {MAX_DATAGRAM_PAYLOAD} bytes)"); + if len > MAX_FZ_PAYLOAD { + telemetry_event!(from = %d.from, %len, "Dropping too large datagram (max allowed: {MAX_FZ_PAYLOAD} bytes)"); return false; } diff --git a/rust/connlib/tunnel/src/p2p_control.rs b/rust/connlib/tunnel/src/p2p_control.rs index d9ebe2741..f756a87d9 100644 --- a/rust/connlib/tunnel/src/p2p_control.rs +++ b/rust/connlib/tunnel/src/p2p_control.rs @@ -129,7 +129,7 @@ pub mod dns_resource_nat { let serialized = serde_json::to_vec(&assigned_ips).unwrap(); assert_eq!(serialized.len(), 402); - assert!(serialized.len() <= ip_packet::PACKET_SIZE); + assert!(serialized.len() <= ip_packet::MAX_IP_SIZE); } #[test] diff --git a/rust/connlib/tunnel/src/tests/dns_server_resource.rs b/rust/connlib/tunnel/src/tests/dns_server_resource.rs index 67796ab54..6308fffbf 100644 --- a/rust/connlib/tunnel/src/tests/dns_server_resource.rs +++ b/rust/connlib/tunnel/src/tests/dns_server_resource.rs @@ -8,7 +8,7 @@ use domain::base::{ iana::{Class, Rcode}, Message, MessageBuilder, Record, RecordData, ToName, Ttl, }; -use ip_packet::{IpPacket, MAX_DATAGRAM_PAYLOAD}; +use ip_packet::{IpPacket, MAX_UDP_PAYLOAD}; use super::dns_records::DnsRecords; @@ -102,12 +102,12 @@ fn handle_dns_query(query: &Message<[u8]>, global_dns_records: &DnsRecords) -> M fn truncate_dns_response(message: Message>) -> Vec { let mut message_bytes = message.as_octets().to_vec(); - if message_bytes.len() > MAX_DATAGRAM_PAYLOAD { + if message_bytes.len() > MAX_UDP_PAYLOAD { let mut new_message = message.clone(); new_message.header_mut().set_tc(true); let message_truncation = match message.answer() { - Ok(answer) if answer.pos() <= MAX_DATAGRAM_PAYLOAD => answer.pos(), + Ok(answer) if answer.pos() <= MAX_UDP_PAYLOAD => answer.pos(), // This should be very unlikely or impossible. _ => message.question().pos(), }; diff --git a/rust/dns-over-tcp/src/stub_device.rs b/rust/dns-over-tcp/src/stub_device.rs index 766245eed..61c41a1b8 100644 --- a/rust/dns-over-tcp/src/stub_device.rs +++ b/rust/dns-over-tcp/src/stub_device.rs @@ -47,7 +47,7 @@ impl smoltcp::phy::Device for InMemoryDevice { fn capabilities(&self) -> smoltcp::phy::DeviceCapabilities { let mut caps = smoltcp::phy::DeviceCapabilities::default(); caps.medium = smoltcp::phy::Medium::Ip; - caps.max_transmission_unit = ip_packet::PACKET_SIZE; + caps.max_transmission_unit = ip_packet::MAX_IP_SIZE; caps } @@ -62,7 +62,7 @@ impl smoltcp::phy::TxToken for SmolTxToken<'_> { where F: FnOnce(&mut [u8]) -> R, { - let max_len = ip_packet::PACKET_SIZE; + let max_len = ip_packet::MAX_IP_SIZE; if len > max_len { tracing::warn!("Packets larger than {max_len} are not supported; len={len}"); diff --git a/rust/gateway/src/main.rs b/rust/gateway/src/main.rs index ed70cf0d4..74bd6f74a 100644 --- a/rust/gateway/src/main.rs +++ b/rust/gateway/src/main.rs @@ -106,7 +106,7 @@ async fn try_main(cli: Cli, telemetry: &mut Telemetry) -> Result { let (sender, receiver) = mpsc::channel::(10); - let mut tun_device_manager = TunDeviceManager::new(ip_packet::PACKET_SIZE, cli.tun_threads)?; + let mut tun_device_manager = TunDeviceManager::new(ip_packet::MAX_IP_SIZE, cli.tun_threads)?; let tun = tun_device_manager.make_tun()?; tunnel.set_tun(Box::new(tun)); diff --git a/rust/headless-client/src/ipc_service.rs b/rust/headless-client/src/ipc_service.rs index cd2e975de..27ea8eb71 100644 --- a/rust/headless-client/src/ipc_service.rs +++ b/rust/headless-client/src/ipc_service.rs @@ -329,7 +329,7 @@ impl<'a> Handler<'a> { .next_client_split() .await .context("Failed to wait for incoming IPC connection from a GUI")?; - let tun_device = TunDeviceManager::new(ip_packet::PACKET_SIZE, crate::NUM_TUN_THREADS)?; + let tun_device = TunDeviceManager::new(ip_packet::MAX_IP_SIZE, crate::NUM_TUN_THREADS)?; Ok(Self { dns_controller, diff --git a/rust/headless-client/src/main.rs b/rust/headless-client/src/main.rs index 17210525b..b0459516e 100644 --- a/rust/headless-client/src/main.rs +++ b/rust/headless-client/src/main.rs @@ -237,7 +237,7 @@ fn main() -> Result<()> { let mut hangup = signals::Hangup::new()?; let mut tun_device = TunDeviceManager::new( - ip_packet::PACKET_SIZE, + ip_packet::MAX_IP_SIZE, firezone_headless_client::NUM_TUN_THREADS, )?; let mut cb_rx = ReceiverStream::new(cb_rx).fuse(); diff --git a/rust/ip-packet/src/buffer_pool.rs b/rust/ip-packet/src/buffer_pool.rs index ed7d8e785..39faeffd1 100644 --- a/rust/ip-packet/src/buffer_pool.rs +++ b/rust/ip-packet/src/buffer_pool.rs @@ -3,13 +3,13 @@ use std::{ sync::{Arc, LazyLock}, }; -use crate::MAX_DATAGRAM_PAYLOAD; +use crate::MAX_FZ_PAYLOAD; type BufferPool = Arc>>; static BUFFER_POOL: LazyLock = LazyLock::new(|| { Arc::new(lockfree_object_pool::MutexObjectPool::new( - || vec![0; MAX_DATAGRAM_PAYLOAD], + || vec![0; MAX_FZ_PAYLOAD], |v| v.fill(0), )) }); @@ -63,7 +63,7 @@ impl Drop for Buffer { fn drop(&mut self) { debug_assert_eq!( self.0.capacity(), - MAX_DATAGRAM_PAYLOAD, + MAX_FZ_PAYLOAD, "Buffer should never re-allocate" ) } diff --git a/rust/ip-packet/src/lib.rs b/rust/ip-packet/src/lib.rs index 304405854..ca80935ee 100644 --- a/rust/ip-packet/src/lib.rs +++ b/rust/ip-packet/src/lib.rs @@ -38,14 +38,32 @@ use tcp_header_slice_mut::TcpHeaderSliceMut; use udp_header_slice_mut::UdpHeaderSliceMut; /// The maximum size of an IP packet we can handle. -pub const PACKET_SIZE: usize = 1280; -/// The maximum payload of a UDP packet that carries an encrypted IP packet. -pub const MAX_DATAGRAM_PAYLOAD: usize = - PACKET_SIZE + WG_OVERHEAD + NAT46_OVERHEAD + DATA_CHANNEL_OVERHEAD; +pub const MAX_IP_SIZE: usize = 1280; +/// The maximum payload an IP packet can have. +/// +/// IPv6 headers are always a fixed size whereas IPv4 headers can vary. +/// The max length of an IPv4 header is > the fixed length of an IPv6 header. +pub const MAX_IP_PAYLOAD: usize = MAX_IP_SIZE - etherparse::Ipv4Header::MAX_LEN; +/// The maximum payload a UDP packet can have. +pub const MAX_UDP_PAYLOAD: usize = MAX_IP_PAYLOAD - etherparse::UdpHeader::LEN; + +/// The maximum size of the payload that Firezone will send between nodes. +/// +/// - The TUN device MTU is constrained to 1280 ([`MAX_IP_SIZE`]). +/// - WireGuard adds an overhoad of 32 bytes ([`WG_OVERHEAD`]). +/// - In case NAT46 comes into effect, the size may increase by 20 ([`NAT46_OVERHEAD`]). +/// - In case the connection is relayed, a 4 byte overhead is added ([`DATA_CHANNEL_OVERHEAD`]). +/// +/// There is only a single scenario within which all of these apply at once: +/// A client receiving a relayed IPv6 packet from a Gateway from an IPv4-only DNS resource where the sender (i.e. the resource) maxed out the MTU (1280). +/// In that case, the Gateway needs to translate the packet to IPv6, thus increasing the header size by 20 bytes. +/// WireGuard adds its fixed 32-byte overhead and the relayed connections adds its 4 byte overhead. +pub const MAX_FZ_PAYLOAD: usize = + MAX_IP_SIZE + WG_OVERHEAD + NAT46_OVERHEAD + DATA_CHANNEL_OVERHEAD; /// Wireguard has a 32-byte overhead (4b message type + 4b receiver idx + 8b packet counter + 16b AEAD tag) const WG_OVERHEAD: usize = 32; /// In order to do NAT46 without copying, we need 20 extra byte in the buffer (IPv6 packets are 20 byte bigger than IPv4). -pub const NAT46_OVERHEAD: usize = 20; +pub(crate) const NAT46_OVERHEAD: usize = 20; /// TURN's data channels have a 4 byte overhead. const DATA_CHANNEL_OVERHEAD: usize = 4; @@ -331,7 +349,7 @@ pub fn ipv6_translated(ip: Ipv6Addr) -> Option { impl IpPacket { pub fn new(buf: IpPacketBuf, len: usize) -> Result { - anyhow::ensure!(len <= PACKET_SIZE, "Packet too large (len: {len})"); + anyhow::ensure!(len <= MAX_IP_SIZE, "Packet too large (len: {len})"); Ok(match buf.inner[NAT46_OVERHEAD] >> 4 { 4 => IpPacket::Ipv4(ConvertibleIpv4Packet::new(buf, len)?), diff --git a/rust/ip-packet/src/make.rs b/rust/ip-packet/src/make.rs index 0f17b5595..d8ed05b39 100644 --- a/rust/ip-packet/src/make.rs +++ b/rust/ip-packet/src/make.rs @@ -27,7 +27,7 @@ macro_rules! build { pub fn fz_p2p_control(header: [u8; 8], control_payload: &[u8]) -> Result { let ip_payload_size = header.len() + control_payload.len(); - anyhow::ensure!(ip_payload_size <= crate::PACKET_SIZE); + anyhow::ensure!(ip_payload_size <= crate::MAX_IP_SIZE); let builder = etherparse::PacketBuilder::ipv6( crate::fz_p2p_control::ADDR.octets(),