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: replace panics with Result for IP packets (#7135)

Browse Source 
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.
This commit is contained in:
Thomas Eizinger
2024-10-24 03:25:12 +11:00
committed by GitHub
parent ee30368970
commit 6eecfc0cfb
4 changed files with 50 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

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

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

@@ -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<IpPacke
crate::fz_p2p_control::IP_NUMBER,
&payload_buf,
)
.expect("Buffer should be big enough");
.with_context(|| {
format!("Buffer should be big enough; ip_payload_size={ip_payload_size}")
})?;
let ip_packet = IpPacket::new(packet_buf, packet_size).context("Unable to create IP packet")?;
Ok(ip_packet)
@@ -56,21 +62,21 @@ pub fn icmp_request_packet(
seq: u16,
identifier: u16,
payload: &[u8],
) -> Result<IpPacket, IpVersionMismatch> {
) -> Result<IpPacket> {
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<IpPacket, IpVersionMismatch> {
) -> Result<IpPacket> {
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<IP>(
sport: u16,
dport: u16,
payload: Vec<u8>,
) -> Result<IpPacket, IpVersionMismatch>
) -> Result<IpPacket>
where
IP: Into<IpAddr>,
{
@@ -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<IP>(
sport: u16,
dport: u16,
payload: Vec<u8>,
) -> Result<IpPacket, IpVersionMismatch>
) -> Result<IpPacket>
where
IP: Into<IpAddr>,
{
@@ -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),
}
}

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

@@ -24,6 +24,7 @@ fn tcp_packet_v4() -> impl Strategy<Value = IpPacket> {
payload
)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn tcp_packet_v6() -> impl Strategy<Value = IpPacket> {
@@ -40,6 +41,7 @@ fn tcp_packet_v6() -> impl Strategy<Value = IpPacket> {
payload
)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn udp_packet_v4() -> impl Strategy<Value = IpPacket> {
@@ -56,6 +58,7 @@ fn udp_packet_v4() -> impl Strategy<Value = IpPacket> {
payload
)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn udp_packet_v6() -> impl Strategy<Value = IpPacket> {
@@ -72,6 +75,7 @@ fn udp_packet_v6() -> impl Strategy<Value = IpPacket> {
payload
)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn icmp_request_packet_v4() -> impl Strategy<Value = IpPacket> {
@@ -96,6 +100,7 @@ fn icmp_request_packet_v4() -> impl Strategy<Value = IpPacket> {
build!(packet, EMPTY_PAYLOAD)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn icmp_reply_packet_v4() -> impl Strategy<Value = IpPacket> {
@@ -120,6 +125,7 @@ fn icmp_reply_packet_v4() -> impl Strategy<Value = IpPacket> {
build!(packet, EMPTY_PAYLOAD)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn icmp_request_packet_v6() -> impl Strategy<Value = IpPacket> {
@@ -135,6 +141,7 @@ fn icmp_request_packet_v6() -> impl Strategy<Value = IpPacket> {
EMPTY_PAYLOAD
)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn icmp_reply_packet_v6() -> impl Strategy<Value = IpPacket> {
@@ -150,6 +157,7 @@ fn icmp_reply_packet_v6() -> impl Strategy<Value = IpPacket> {
EMPTY_PAYLOAD
)
})
.prop_map(|r: anyhow::Result<IpPacket>| r.unwrap())
}
fn ipv4_options() -> impl Strategy<Value = Ipv4Options> {

14
rust/logging/src/log_unwrap.rs
View File

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