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

fix(connlib): use correct constant for truncating DNS responses (#7551)

Browse Source 
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.
This commit is contained in:
Thomas Eizinger
2024-12-19 18:15:43 +01:00
committed by GitHub
parent af1834d0e5
commit bc2febed99
12 changed files with 63 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
rust/connlib/snownet/src/node.rs
View File

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

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

@@ -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<Vec<u8>>,
message: Message<Vec<u8>>,
) -> 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<u8>>) -> Vec<u8> {
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<u8>>) -> Vec<u8> {
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
}

6
rust/connlib/tunnel/src/io.rs
View File

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

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

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

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

@@ -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<u8>>) -> Vec<u8> {
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(),
};

4
rust/dns-over-tcp/src/stub_device.rs
View File

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

2
rust/gateway/src/main.rs
View File

@@ -106,7 +106,7 @@ async fn try_main(cli: Cli, telemetry: &mut Telemetry) -> Result<ExitCode> {
let (sender, receiver) = mpsc::channel::<Interface>(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));

2
rust/headless-client/src/ipc_service.rs
View File

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

2
rust/headless-client/src/main.rs
View File

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

6
rust/ip-packet/src/buffer_pool.rs
View File

@@ -3,13 +3,13 @@ use std::{
sync::{Arc, LazyLock},
};
use crate::MAX_DATAGRAM_PAYLOAD;
use crate::MAX_FZ_PAYLOAD;
type BufferPool = Arc<lockfree_object_pool::MutexObjectPool<Vec<u8>>>;
static BUFFER_POOL: LazyLock<BufferPool> = 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"
)
}

30
rust/ip-packet/src/lib.rs
View File

@@ -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<Ipv4Addr> {
impl IpPacket {
pub fn new(buf: IpPacketBuf, len: usize) -> Result<Self> {
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)?),

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

@@ -27,7 +27,7 @@ macro_rules! build {
pub fn fz_p2p_control(header: [u8; 8], control_payload: &[u8]) -> Result<IpPacket> {
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(),