From f41630dedfb483f7a4d075cad8ee505cbe000a73 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 28 Aug 2026 17:15:01 -0600 Subject: [PATCH] fix(net): Bound the transport checksum by the network header's length An ethernet frame shorter than 60 octets arrives padded, and that padding sat inside the slice we summed. TCP and ICMPv6 take their pseudo header length from the slice, so a bare 54-octet ACK left with a checksum off by the pad length. This is wrong even when the padding is zeroed, which is why UDP and ICMPv4 usually survived it and TCP did not. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Daniel Noland --- net/src/headers/mod.rs | 32 +++++++++ net/src/packet/mod.rs | 155 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 186 insertions(+), 1 deletion(-) diff --git a/net/src/headers/mod.rs b/net/src/headers/mod.rs index 22fb5237ca..e715486a10 100644 --- a/net/src/headers/mod.rs +++ b/net/src/headers/mod.rs @@ -890,7 +890,39 @@ impl Headers { } } + /// The number of octets which follow the transport header according to the network header. + /// + /// A frame can carry more octets than its network header accounts for: ethernet pads short + /// frames out to 60 octets (`IEEE 802.3` clause 4.2.3.3), and some devices append trailers. + /// Those octets belong to no upper layer, so a caller holding a whole frame needs this to find + /// where the transport payload ends before computing a checksum over it. + /// + /// # Returns + /// + /// Returns `None` when the network or transport header is absent, or when the network header + /// claims fewer octets than the headers which follow it occupy -- a malformed packet, for which + /// no honest payload length exists. + pub(crate) fn transport_payload_len(&self) -> Option { + let ip_payload_len = match self.net.as_ref()? { + Net::Ipv4(ip) => usize::from(ip.0.payload_len().ok()?), + Net::Ipv6(ip) => usize::from(ip.0.payload_length), + }; + let after_net = self + .net_ext + .iter() + .map(|ext| usize::from(ext.size().get())) + .sum::() + + usize::from(self.transport.as_ref()?.size().get()); + ip_payload_len.checked_sub(after_net) + } + /// update the checksums of the headers + /// + /// `payload` must be exactly the octets which follow the transport header, and no more: it is + /// summed in full. A caller working from a frame buffer should bound it with + /// [`Headers::transport_payload_len`] first, as [`Packet::update_checksums`] does. + /// + /// [`Packet::update_checksums`]: crate::packet::Packet::update_checksums pub(crate) fn update_checksums(&mut self, payload: impl AsRef<[u8]>) { let is_vxlan = self.try_vxlan().is_some(); diff --git a/net/src/packet/mod.rs b/net/src/packet/mod.rs index 3cdee54bf3..d56025da4e 100644 --- a/net/src/packet/mod.rs +++ b/net/src/packet/mod.rs @@ -326,8 +326,22 @@ impl Packet { } /// Update the network and transport checksums based on the current headers. + /// + /// Only the octets the network header vouches for are summed. A frame can hold more than + /// that: ethernet pads short frames out to 60 octets, and some devices append trailers. Those + /// octets are not payload, and including them corrupts the result -- the pseudo header length + /// comes out too large for TCP and `ICMPv6`, and any non-zero octet perturbs the sum for every + /// protocol. + /// + /// A buffer holding *less* than the network header claims is left as-is: the packet is + /// truncated, and no choice of payload yields a checksum the far end will accept. pub fn update_checksums(&mut self) -> &mut Self { - self.headers.update_checksums(&self.payload); + let payload = self.payload.as_ref(); + let payload = match self.headers.transport_payload_len() { + Some(len) if len <= payload.len() => &payload[..len], + _ => payload, + }; + self.headers.update_checksums(payload); self.meta_mut().set_checksum_refresh(false); self } @@ -888,3 +902,142 @@ mod qos_roundtrip_tests { } } } + +#[cfg(test)] +mod padding_tests { + use crate::buffer::TestBuffer; + use crate::checksum::Checksum; + use crate::headers::{TryHeaders, TryIcmp4, TryIp, TryTcp, TryUdp}; + use crate::packet::Packet; + use crate::tcp::TcpChecksumPayload; + use crate::udp::UdpChecksumPayload; + + /// The smallest frame ethernet will carry, per `IEEE 802.3` clause 4.2.3.3. + const MIN_ETHERNET_FRAME: usize = 60; + + /// An ethernet + IPv4 frame carrying `l4`, with the IPv4 total length describing `l4` exactly. + fn frame(protocol: u8, l4: &[u8]) -> Vec { + let mut frame = Vec::new(); + frame.extend_from_slice(&[0x02, 0, 0, 0, 0, 1]); // destination mac + frame.extend_from_slice(&[0x02, 0, 0, 0, 0, 2]); // source mac + frame.extend_from_slice(&[0x08, 0x00]); // ipv4 + frame.extend_from_slice(&[0x45, 0x00]); + #[allow(clippy::cast_possible_truncation)] // test input is small + frame.extend_from_slice(&((20 + l4.len()) as u16).to_be_bytes()); + frame.extend_from_slice(&[0x00, 0x01, 0x00, 0x00, 0x40, protocol, 0x00, 0x00]); + frame.extend_from_slice(&[192, 168, 0, 1]); // source ip + frame.extend_from_slice(&[192, 168, 0, 2]); // destination ip + frame.extend_from_slice(l4); + frame + } + + /// A bare TCP acknowledgement: no options, no payload, 20 octets. + fn tcp_ack() -> Vec { + let mut tcp = Vec::new(); + tcp.extend_from_slice(&1000_u16.to_be_bytes()); // source port + tcp.extend_from_slice(&2000_u16.to_be_bytes()); // destination port + tcp.extend_from_slice(&[0, 0, 0, 1]); // sequence number + tcp.extend_from_slice(&[0, 0, 0, 2]); // acknowledgement number + tcp.extend_from_slice(&[0x50, 0x10]); // data offset 5, ACK + tcp.extend_from_slice(&1024_u16.to_be_bytes()); // window + tcp.extend_from_slice(&[0, 0]); // checksum + tcp.extend_from_slice(&[0, 0]); // urgent pointer + tcp + } + + /// A UDP datagram header describing `payload`. + fn udp(payload: &[u8]) -> Vec { + let mut udp = Vec::new(); + udp.extend_from_slice(&1000_u16.to_be_bytes()); // source port + udp.extend_from_slice(&2000_u16.to_be_bytes()); // destination port + #[allow(clippy::cast_possible_truncation)] // test input is small + udp.extend_from_slice(&((8 + payload.len()) as u16).to_be_bytes()); + udp.extend_from_slice(&[0, 0]); // checksum + udp.extend_from_slice(payload); + udp + } + + /// An `ICMPv4` echo request, 8 octets. + fn icmp4_echo_request() -> Vec { + vec![8, 0, 0, 0, 0x00, 0x2a, 0x00, 0x01] + } + + fn parse(frame: &[u8]) -> Packet { + Packet::new(TestBuffer::from_raw_data(frame)).expect("frame does not parse") + } + + /// Pad `frame` out to the ethernet minimum with `filler`. + /// + /// `IEEE 802.3` requires the padding but does not constrain its content, so a peer may send + /// anything here. Historically it has sent the contents of uninitialized memory + /// (`CVE-2003-0001`). + fn pad(mut frame: Vec, filler: u8) -> Vec { + assert!(frame.len() < MIN_ETHERNET_FRAME, "frame needs no padding"); + frame.resize(MIN_ETHERNET_FRAME, filler); + frame + } + + #[test] + fn tcp_checksum_excludes_zeroed_ethernet_padding() { + let mut packet = parse(&pad(frame(6, &tcp_ack()), 0)); + packet.update_checksums(); + let net = packet.headers().try_ip().expect("no ip header").clone(); + packet + .headers() + .try_tcp() + .expect("no tcp header") + .validate_checksum(&TcpChecksumPayload::new(&net, &[])) + .expect("padding leaked into the tcp checksum"); + } + + #[test] + fn udp_checksum_excludes_non_zero_ethernet_padding() { + let mut packet = parse(&pad(frame(17, &udp(&[])), 0xab)); + packet.update_checksums(); + let net = packet.headers().try_ip().expect("no ip header").clone(); + packet + .headers() + .try_udp() + .expect("no udp header") + .validate_checksum(&UdpChecksumPayload::new(&net, &[])) + .expect("padding leaked into the udp checksum"); + } + + #[test] + fn icmp4_checksum_excludes_non_zero_ethernet_padding() { + let mut packet = parse(&pad(frame(1, &icmp4_echo_request()), 0xab)); + packet.update_checksums(); + packet + .headers() + .try_icmp4() + .expect("no icmp header") + .validate_checksum(&[]) + .expect("padding leaked into the icmp checksum"); + } + + #[test] + fn checksum_still_covers_a_real_payload() { + let payload: Vec = (0..32_u8).collect(); + let mut packet = parse(&frame(17, &udp(&payload))); + assert_eq!(packet.payload().as_ref(), payload.as_slice()); + packet.update_checksums(); + let net = packet.headers().try_ip().expect("no ip header").clone(); + packet + .headers() + .try_udp() + .expect("no udp header") + .validate_checksum(&UdpChecksumPayload::new(&net, &payload)) + .expect("payload dropped out of the udp checksum"); + } + + /// A payload shorter than the network header claims cannot be checksummed correctly by anyone. + /// All this asks is that we compute *something* rather than panic on the short slice. + #[test] + fn truncated_payload_does_not_panic() { + let payload: Vec = (0..32_u8).collect(); + let mut frame = frame(17, &udp(&payload)); + frame.truncate(frame.len() - 8); + let mut packet = parse(&frame); + packet.update_checksums(); + } +}