From 6eecfc0cfb51eb2719470aad1731e9f7781cf7cd Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 24 Oct 2024 03:25:12 +1100 Subject: [PATCH] fix: replace panics with `Result` for IP packets (#7135) My theory for this issue is that we receive a UDP DNS response from an upstream server that is bigger than our MTU and thus forwarding it fails. This PR doesn't fix that issue by itself but only mitigates the actual panic. To properly fix the underlying issue, we need to parse the DNS message. Truncate it and set the TC bit. Related: #7121. --- rust/connlib/tunnel/src/client.rs | 4 +-- rust/ip-packet/src/make.rs | 46 +++++++++++++++++-------------- rust/ip-packet/src/proptests.rs | 8 ++++++ rust/logging/src/log_unwrap.rs | 14 ++++++++++ 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index e0d1479c6..ab564160b 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -385,7 +385,7 @@ impl ClientState { }); self.try_queue_udp_dns_response(server, source, &message) - .log_unwrap_debug("Failed to queue UDP DNS response"); + .log_unwrap_warn("Failed to queue UDP DNS response"); } (dns::Transport::Tcp { source }, result) => { let message = result @@ -400,7 +400,7 @@ impl ClientState { self.tcp_dns_server .send_message(source, message) - .log_unwrap_debug("Failed to send TCP DNS response"); + .log_unwrap_warn("Failed to send TCP DNS response"); } } } diff --git a/rust/ip-packet/src/make.rs b/rust/ip-packet/src/make.rs index ccd2a7b27..a110ef157 100644 --- a/rust/ip-packet/src/make.rs +++ b/rust/ip-packet/src/make.rs @@ -1,7 +1,7 @@ //! Factory module for making all kinds of packets. use crate::{IpPacket, IpPacketBuf}; -use anyhow::{Context, Result}; +use anyhow::{bail, Context as _, Result}; use etherparse::PacketBuilder; use std::net::IpAddr; @@ -9,14 +9,18 @@ use std::net::IpAddr; #[macro_export] macro_rules! build { ($packet:expr, $payload:ident) => {{ + use ::anyhow::Context as _; + let size = $packet.size($payload.len()); let mut ip = $crate::IpPacketBuf::new(); $packet .write(&mut std::io::Cursor::new(ip.buf()), &$payload) - .expect("Buffer should be big enough"); + .with_context(|| format!("Payload is too big; len={size}"))?; - IpPacket::new(ip, size).expect("Should be a valid IP packet") + let packet = IpPacket::new(ip, size).context("Failed to create IP packet")?; + + Ok(packet) }}; } @@ -44,7 +48,9 @@ pub fn fz_p2p_control(header: [u8; 8], control_payload: &[u8]) -> Result Result { +) -> Result { match (src, dst.into()) { (IpAddr::V4(src), IpAddr::V4(dst)) => { let packet = PacketBuilder::ipv4(src.octets(), dst.octets(), 64) .icmpv4_echo_request(identifier, seq); - Ok(build!(packet, payload)) + build!(packet, payload) } (IpAddr::V6(src), IpAddr::V6(dst)) => { let packet = PacketBuilder::ipv6(src.octets(), dst.octets(), 64) .icmpv6_echo_request(identifier, seq); - Ok(build!(packet, payload)) + build!(packet, payload) } - _ => Err(IpVersionMismatch), + _ => bail!(IpVersionMismatch), } } @@ -80,21 +86,21 @@ pub fn icmp_reply_packet( seq: u16, identifier: u16, payload: &[u8], -) -> Result { +) -> Result { match (src, dst.into()) { (IpAddr::V4(src), IpAddr::V4(dst)) => { let packet = PacketBuilder::ipv4(src.octets(), dst.octets(), 64) .icmpv4_echo_reply(identifier, seq); - Ok(build!(packet, payload)) + build!(packet, payload) } (IpAddr::V6(src), IpAddr::V6(dst)) => { let packet = PacketBuilder::ipv6(src.octets(), dst.octets(), 64) .icmpv6_echo_reply(identifier, seq); - Ok(build!(packet, payload)) + build!(packet, payload) } - _ => Err(IpVersionMismatch), + _ => bail!(IpVersionMismatch), } } @@ -134,7 +140,7 @@ pub fn tcp_packet( sport: u16, dport: u16, payload: Vec, -) -> Result +) -> Result where IP: Into, { @@ -143,15 +149,15 @@ where let packet = PacketBuilder::ipv4(src.octets(), dst.octets(), 64).tcp(sport, dport, 0, 128); - Ok(build!(packet, payload)) + build!(packet, payload) } (IpAddr::V6(src), IpAddr::V6(dst)) => { let packet = PacketBuilder::ipv6(src.octets(), dst.octets(), 64).tcp(sport, dport, 0, 128); - Ok(build!(packet, payload)) + build!(packet, payload) } - _ => Err(IpVersionMismatch), + _ => bail!(IpVersionMismatch), } } @@ -161,7 +167,7 @@ pub fn udp_packet( sport: u16, dport: u16, payload: Vec, -) -> Result +) -> Result where IP: Into, { @@ -169,14 +175,14 @@ where (IpAddr::V4(src), IpAddr::V4(dst)) => { let packet = PacketBuilder::ipv4(src.octets(), dst.octets(), 64).udp(sport, dport); - Ok(build!(packet, payload)) + build!(packet, payload) } (IpAddr::V6(src), IpAddr::V6(dst)) => { let packet = PacketBuilder::ipv6(src.octets(), dst.octets(), 64).udp(sport, dport); - Ok(build!(packet, payload)) + build!(packet, payload) } - _ => Err(IpVersionMismatch), + _ => bail!(IpVersionMismatch), } } diff --git a/rust/ip-packet/src/proptests.rs b/rust/ip-packet/src/proptests.rs index e45b2808f..7800451bc 100644 --- a/rust/ip-packet/src/proptests.rs +++ b/rust/ip-packet/src/proptests.rs @@ -24,6 +24,7 @@ fn tcp_packet_v4() -> impl Strategy { payload ) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn tcp_packet_v6() -> impl Strategy { @@ -40,6 +41,7 @@ fn tcp_packet_v6() -> impl Strategy { payload ) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn udp_packet_v4() -> impl Strategy { @@ -56,6 +58,7 @@ fn udp_packet_v4() -> impl Strategy { payload ) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn udp_packet_v6() -> impl Strategy { @@ -72,6 +75,7 @@ fn udp_packet_v6() -> impl Strategy { payload ) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn icmp_request_packet_v4() -> impl Strategy { @@ -96,6 +100,7 @@ fn icmp_request_packet_v4() -> impl Strategy { build!(packet, EMPTY_PAYLOAD) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn icmp_reply_packet_v4() -> impl Strategy { @@ -120,6 +125,7 @@ fn icmp_reply_packet_v4() -> impl Strategy { build!(packet, EMPTY_PAYLOAD) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn icmp_request_packet_v6() -> impl Strategy { @@ -135,6 +141,7 @@ fn icmp_request_packet_v6() -> impl Strategy { EMPTY_PAYLOAD ) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn icmp_reply_packet_v6() -> impl Strategy { @@ -150,6 +157,7 @@ fn icmp_reply_packet_v6() -> impl Strategy { EMPTY_PAYLOAD ) }) + .prop_map(|r: anyhow::Result| r.unwrap()) } fn ipv4_options() -> impl Strategy { diff --git a/rust/logging/src/log_unwrap.rs b/rust/logging/src/log_unwrap.rs index c2bd9efef..f0396e1fb 100644 --- a/rust/logging/src/log_unwrap.rs +++ b/rust/logging/src/log_unwrap.rs @@ -1,6 +1,8 @@ use std::error::Error; pub trait LogUnwrap { + #[track_caller] + fn log_unwrap_warn(&self, msg: &str); #[track_caller] fn log_unwrap_debug(&self, msg: &str); #[track_caller] @@ -8,6 +10,18 @@ pub trait LogUnwrap { } impl LogUnwrap for anyhow::Result<()> { + #[track_caller] + fn log_unwrap_warn(&self, msg: &str) { + match self { + Ok(()) => {} + Err(e) => { + let error: &dyn Error = e.as_ref(); + + tracing::warn!(error, "{msg}") + } + } + } + #[track_caller] fn log_unwrap_debug(&self, msg: &str) { match self {