From 0e20f7d086dddbf83dd21fff733b1feaae250883 Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Tue, 12 Nov 2024 19:46:32 +0000
Subject: [PATCH] chore(connlib): better error reporting for invalid IP packets
 (#7320)

Currently, we don't report very detailed errors when we fail to parse
certain IP packets. With this patch, we use `Result` in more places and
also extend the validation of IP packets to:

a) enforce a length of at most 1280 bytes. This should already be the
case due to our MTU but bad things may happen if that is off for some
reason
b) validate the entire IP packet instead of just its header
---
 rust/connlib/tunnel/src/device_channel.rs |  4 ++--
 rust/dns-over-tcp/src/stub_device.rs      | 11 ++++++---
 rust/ip-packet/src/lib.rs                 | 28 ++++++++++++-----------
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/rust/connlib/tunnel/src/device_channel.rs b/rust/connlib/tunnel/src/device_channel.rs
index 829fc4fd5..bb2c0bea0 100644
--- a/rust/connlib/tunnel/src/device_channel.rs
+++ b/rust/connlib/tunnel/src/device_channel.rs
@@ -47,10 +47,10 @@ impl Device {
             )));
         }
 
-        let packet = IpPacket::new(ip_packet, n).ok_or_else(|| {
+        let packet = IpPacket::new(ip_packet, n).map_err(|e| {
             io::Error::new(
                 io::ErrorKind::InvalidInput,
-                "received bytes are not an IP packet",
+                format!("Failed to parse IP packet: {e:#}"),
             )
         })?;
 
diff --git a/rust/dns-over-tcp/src/stub_device.rs b/rust/dns-over-tcp/src/stub_device.rs
index c099d37da..294b98b14 100644
--- a/rust/dns-over-tcp/src/stub_device.rs
+++ b/rust/dns-over-tcp/src/stub_device.rs
@@ -1,5 +1,6 @@
 use std::collections::VecDeque;
 
+use firezone_logging::anyhow_dyn_err;
 use ip_packet::{IpPacket, IpPacketBuf};
 
 /// A in-memory device for [`smoltcp`] that is entirely backed by buffers.
@@ -73,10 +74,14 @@ impl<'a> smoltcp::phy::TxToken for SmolTxToken<'a> {
         let mut ip_packet_buf = IpPacketBuf::new();
         let result = f(ip_packet_buf.buf());
 
-        let Some(mut ip_packet) = IpPacket::new(ip_packet_buf, len) else {
-            tracing::warn!("Received invalid IP packet");
-            return result;
+        let mut ip_packet = match IpPacket::new(ip_packet_buf, len) {
+            Ok(p) => p,
+            Err(e) => {
+                tracing::warn!(error = anyhow_dyn_err(&e), "Received invalid IP packet");
+                return result;
+            }
         };
+
         ip_packet.update_checksum();
         self.outbound_packets.push_back(ip_packet);
 
diff --git a/rust/ip-packet/src/lib.rs b/rust/ip-packet/src/lib.rs
index 446c12d22..5778af773 100644
--- a/rust/ip-packet/src/lib.rs
+++ b/rust/ip-packet/src/lib.rs
@@ -21,7 +21,7 @@ pub use fz_p2p_control_slice::FzP2pControlSlice;
 #[cfg(all(test, feature = "proptest"))]
 mod proptests;
 
-use anyhow::{Context as _, Result};
+use anyhow::{bail, Context as _, Result};
 use icmpv4_header_slice_mut::Icmpv4HeaderSliceMut;
 use icmpv6_header_slice_mut::Icmpv6EchoHeaderSliceMut;
 use ipv4_header_slice_mut::Ipv4HeaderSliceMut;
@@ -162,15 +162,15 @@ pub struct ConvertibleIpv4Packet {
 }
 
 impl ConvertibleIpv4Packet {
-    pub fn new(ip: IpPacketBuf, len: usize) -> Option<ConvertibleIpv4Packet> {
+    pub fn new(ip: IpPacketBuf, len: usize) -> Result<ConvertibleIpv4Packet> {
         let this = Self {
             buf: ip.inner,
             start: NAT46_OVERHEAD,
             len,
         };
-        Ipv4HeaderSlice::from_slice(this.packet()).ok()?;
+        Ipv4Slice::from_slice(this.packet()).context("Invalid IPv4 packet")?;
 
-        Some(this)
+        Ok(this)
     }
 
     fn ip_header(&self) -> Ipv4HeaderSlice {
@@ -242,16 +242,16 @@ pub struct ConvertibleIpv6Packet {
 }
 
 impl ConvertibleIpv6Packet {
-    pub fn new(ip: IpPacketBuf, len: usize) -> Option<ConvertibleIpv6Packet> {
+    pub fn new(ip: IpPacketBuf, len: usize) -> Result<ConvertibleIpv6Packet> {
         let this = Self {
             buf: ip.inner,
             start: NAT46_OVERHEAD,
             len,
         };
 
-        Ipv6HeaderSlice::from_slice(this.packet()).ok()?;
+        Ipv6Slice::from_slice(this.packet()).context("Invalid IPv6 packet")?;
 
-        Some(this)
+        Ok(this)
     }
 
     fn header(&self) -> Ipv6HeaderSlice {
@@ -323,12 +323,14 @@ pub fn ipv6_translated(ip: Ipv6Addr) -> Option<Ipv4Addr> {
 }
 
 impl IpPacket {
-    pub fn new(buf: IpPacketBuf, len: usize) -> Option<Self> {
-        match buf.inner[NAT46_OVERHEAD] >> 4 {
-            4 => Some(IpPacket::Ipv4(ConvertibleIpv4Packet::new(buf, len)?)),
-            6 => Some(IpPacket::Ipv6(ConvertibleIpv6Packet::new(buf, len)?)),
-            _ => None,
-        }
+    pub fn new(buf: IpPacketBuf, len: usize) -> Result<Self> {
+        anyhow::ensure!(len <= PACKET_SIZE, "Packet too large (len: {len})");
+
+        Ok(match buf.inner[NAT46_OVERHEAD] >> 4 {
+            4 => IpPacket::Ipv4(ConvertibleIpv4Packet::new(buf, len)?),
+            6 => IpPacket::Ipv6(ConvertibleIpv6Packet::new(buf, len)?),
+            v => bail!("Invalid IP version: {v}"),
+        })
     }
 
     pub(crate) fn consume_to_ipv4(self, src: Ipv4Addr, dst: Ipv4Addr) -> Result<IpPacket> {