From 9206e766edc0492a3899633c5ea3a9a8ace12fe0 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Tue, 3 Jun 2025 15:24:31 -0700 Subject: [PATCH] net/packet: cleanup IPv4 fragment guards The first packet fragment guard had an additional guard clause that was incorrectly comparing a length in bytes to a length in octets, and was also comparing what should have been an entire IPv4 through transport header length to a subprotocol payload length. The subprotocol header size guards were otherwise protecting against short transport headers, as is the conservative non-first fragment minimum offset size. Add an explicit disallowing of fragmentation for TSMP for the avoidance of doubt. Updates #cleanup Updates #5727 Signed-off-by: James Tucker --- net/packet/header.go | 1 + net/packet/packet.go | 34 +++++++---- net/packet/packet_test.go | 122 ++++++++++++++++++++++++++++++++++++++ net/packet/tsmp.go | 2 + 4 files changed, 149 insertions(+), 10 deletions(-) diff --git a/net/packet/header.go b/net/packet/header.go index dbe84429a..fa66a8641 100644 --- a/net/packet/header.go +++ b/net/packet/header.go @@ -8,6 +8,7 @@ import ( "math" ) +const igmpHeaderLength = 8 const tcpHeaderLength = 20 const sctpHeaderLength = 12 diff --git a/net/packet/packet.go b/net/packet/packet.go index 876a653ed..34b63aadd 100644 --- a/net/packet/packet.go +++ b/net/packet/packet.go @@ -161,14 +161,8 @@ func (q *Parsed) decode4(b []byte) { if fragOfs == 0 { // This is the first fragment - if moreFrags && len(sub) < minFragBlks { - // Suspiciously short first fragment, dump it. - q.IPProto = unknown - return - } - // otherwise, this is either non-fragmented (the usual case) - // or a big enough initial fragment that we can read the - // whole subprotocol header. + // Every protocol below MUST check that it has at least one entire + // transport header in order to protect against fragment confusion. switch q.IPProto { case ipproto.ICMPv4: if len(sub) < icmp4HeaderLength { @@ -180,6 +174,10 @@ func (q *Parsed) decode4(b []byte) { q.dataofs = q.subofs + icmp4HeaderLength return case ipproto.IGMP: + if len(sub) < igmpHeaderLength { + q.IPProto = unknown + return + } // Keep IPProto, but don't parse anything else // out. return @@ -212,6 +210,15 @@ func (q *Parsed) decode4(b []byte) { q.Dst = withPort(q.Dst, binary.BigEndian.Uint16(sub[2:4])) return case ipproto.TSMP: + // Strictly disallow fragmented TSMP + if moreFrags { + q.IPProto = unknown + return + } + if len(sub) < minTSMPSize { + q.IPProto = unknown + return + } // Inter-tailscale messages. q.dataofs = q.subofs return @@ -224,8 +231,11 @@ func (q *Parsed) decode4(b []byte) { } else { // This is a fragment other than the first one. if fragOfs < minFragBlks { - // First frag was suspiciously short, so we can't - // trust the followup either. + // disallow fragment offsets that are potentially inside of a + // transport header. This is notably asymmetric with the + // first-packet limit, that may allow a first-packet that requires a + // shorter offset than this limit, but without state to tie this + // to the first fragment we can not allow shorter packets. q.IPProto = unknown return } @@ -315,6 +325,10 @@ func (q *Parsed) decode6(b []byte) { q.Dst = withPort(q.Dst, binary.BigEndian.Uint16(sub[2:4])) return case ipproto.TSMP: + if len(sub) < minTSMPSize { + q.IPProto = unknown + return + } // Inter-tailscale messages. q.dataofs = q.subofs return diff --git a/net/packet/packet_test.go b/net/packet/packet_test.go index 4fc804a4f..09c2c101d 100644 --- a/net/packet/packet_test.go +++ b/net/packet/packet_test.go @@ -385,6 +385,124 @@ var sctpDecode = Parsed{ Dst: mustIPPort("100.74.70.3:456"), } +var ipv4ShortFirstFragmentBuffer = []byte{ + // IP header (20 bytes) + 0x45, 0x00, 0x00, 0x4f, // Total length 79 bytes + 0x00, 0x01, 0x20, 0x00, // ID, Flags (MoreFragments set, offset 0) + 0x40, 0x06, 0x00, 0x00, // TTL, Protocol (TCP), Checksum + 0x01, 0x02, 0x03, 0x04, // Source IP + 0x05, 0x06, 0x07, 0x08, // Destination IP + // TCP header (20 bytes), but packet is truncated to 59 bytes of TCP data + // (total 79 bytes, 20 for IP) + 0x00, 0x7b, 0x02, 0x37, 0x00, 0x00, 0x12, 0x34, 0x00, 0x00, 0x00, 0x00, + 0x50, 0x12, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, + // Payload (39 bytes) + 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, + 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, + 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, +} + +var ipv4ShortFirstFragmentDecode = Parsed{ + b: ipv4ShortFirstFragmentBuffer, + subofs: 20, + dataofs: 40, + length: len(ipv4ShortFirstFragmentBuffer), + IPVersion: 4, + IPProto: ipproto.TCP, + Src: mustIPPort("1.2.3.4:123"), + Dst: mustIPPort("5.6.7.8:567"), + TCPFlags: 0x12, // SYN + ACK +} + +var ipv4SmallOffsetFragmentBuffer = []byte{ + // IP header (20 bytes) + 0x45, 0x00, 0x00, 0x28, // Total length 40 bytes + 0x00, 0x01, 0x20, 0x08, // ID, Flags (MoreFragments set, offset 8 bytes (0x08 / 8 = 1)) + 0x40, 0x06, 0x00, 0x00, // TTL, Protocol (TCP), Checksum + 0x01, 0x02, 0x03, 0x04, // Source IP + 0x05, 0x06, 0x07, 0x08, // Destination IP + // Payload (20 bytes) - this would be part of the TCP header in a real scenario + 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, 0x61, + 0x61, 0x61, 0x61, 0x61, +} + +var ipv4SmallOffsetFragmentDecode = Parsed{ + b: ipv4SmallOffsetFragmentBuffer, + subofs: 20, // subofs will still be set based on IHL + dataofs: 0, // It's unknown, so dataofs should be 0 + length: len(ipv4SmallOffsetFragmentBuffer), + IPVersion: 4, + IPProto: ipproto.Unknown, // Expected to be Unknown + Src: mustIPPort("1.2.3.4:0"), + Dst: mustIPPort("5.6.7.8:0"), +} + +// First fragment packet missing exactly one byte of the TCP header +var ipv4OneByteShortTCPHeaderBuffer = []byte{ + // IP header (20 bytes) + 0x45, 0x00, 0x00, 0x27, // Total length 51 bytes (20 IP + 19 TCP) + 0x00, 0x01, 0x20, 0x00, // ID, Flags (MoreFragments set, offset 0) + 0x40, 0x06, 0x00, 0x00, // TTL, Protocol (TCP), Checksum + 0x01, 0x02, 0x03, 0x04, // Source IP + 0x05, 0x06, 0x07, 0x08, // Destination IP + // TCP header - only 19 bytes (one byte short of the required 20) + 0x00, 0x7b, 0x02, 0x37, // Source port, Destination port + 0x00, 0x00, 0x12, 0x34, // Sequence number + 0x00, 0x00, 0x00, 0x00, // Acknowledgment number + 0x50, 0x12, 0x01, 0x00, // Data offset, flags, window size + 0x00, 0x00, 0x00, // Checksum (missing the last byte of urgent pointer) +} + +// IPv4 packet with maximum header length (60 bytes = 15 words) and a TCP header that's +// one byte short of being complete +var ipv4MaxHeaderShortTCPBuffer = []byte{ + // IP header with max options (60 bytes) + 0x4F, 0x00, 0x00, 0x4F, // Version (4) + IHL (15), ToS, Total length 79 bytes (60 IP + 19 TCP) + 0x00, 0x01, 0x20, 0x00, // ID, Flags (MoreFragments set, offset 0) + 0x40, 0x06, 0x00, 0x00, // TTL, Protocol (TCP), Checksum + 0x01, 0x02, 0x03, 0x04, // Source IP + 0x05, 0x06, 0x07, 0x08, // Destination IP + // IPv4 options (40 bytes) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + 0x01, 0x01, 0x01, 0x01, // 4 NOP options (padding) + // TCP header - only 19 bytes (one byte short of the required 20) + 0x00, 0x7b, 0x02, 0x37, // Source port, Destination port + 0x00, 0x00, 0x12, 0x34, // Sequence number + 0x00, 0x00, 0x00, 0x00, // Acknowledgment number + 0x50, 0x12, 0x01, 0x00, // Data offset, flags, window size + 0x00, 0x00, 0x00, // Checksum (missing the last byte of urgent pointer) +} + +var ipv4MaxHeaderShortTCPDecode = Parsed{ + b: ipv4MaxHeaderShortTCPBuffer, + subofs: 60, // 60 bytes for full IPv4 header with max options + dataofs: 0, // It's unknown, so dataofs should be 0 + length: len(ipv4MaxHeaderShortTCPBuffer), + IPVersion: 4, + IPProto: ipproto.Unknown, // Expected to be Unknown + Src: mustIPPort("1.2.3.4:0"), + Dst: mustIPPort("5.6.7.8:0"), +} + +var ipv4OneByteShortTCPHeaderDecode = Parsed{ + b: ipv4OneByteShortTCPHeaderBuffer, + subofs: 20, + dataofs: 0, // It's unknown, so dataofs should be 0 + length: len(ipv4OneByteShortTCPHeaderBuffer), + IPVersion: 4, + IPProto: ipproto.Unknown, // Expected to be Unknown + Src: mustIPPort("1.2.3.4:0"), + Dst: mustIPPort("5.6.7.8:0"), +} + func TestParsedString(t *testing.T) { tests := []struct { name string @@ -450,6 +568,10 @@ func TestDecode(t *testing.T) { {"ipv4_sctp", sctpBuffer, sctpDecode}, {"ipv4_frag", tcp4MediumFragmentBuffer, tcp4MediumFragmentDecode}, {"ipv4_fragtooshort", tcp4ShortFragmentBuffer, tcp4ShortFragmentDecode}, + {"ipv4_short_first_fragment", ipv4ShortFirstFragmentBuffer, ipv4ShortFirstFragmentDecode}, + {"ipv4_small_offset_fragment", ipv4SmallOffsetFragmentBuffer, ipv4SmallOffsetFragmentDecode}, + {"ipv4_one_byte_short_tcp_header", ipv4OneByteShortTCPHeaderBuffer, ipv4OneByteShortTCPHeaderDecode}, + {"ipv4_max_header_short_tcp", ipv4MaxHeaderShortTCPBuffer, ipv4MaxHeaderShortTCPDecode}, {"ip97", mustHexDecode("4500 0019 d186 4000 4061 751d 644a 4603 6449 e549 6865 6c6c 6f"), Parsed{ IPVersion: 4, diff --git a/net/packet/tsmp.go b/net/packet/tsmp.go index 4e004cca2..d78d10d36 100644 --- a/net/packet/tsmp.go +++ b/net/packet/tsmp.go @@ -19,6 +19,8 @@ import ( "tailscale.com/types/ipproto" ) +const minTSMPSize = 7 // the rejected body is 7 bytes + // TailscaleRejectedHeader is a TSMP message that says that one // Tailscale node has rejected the connection from another. Unlike a // TCP RST, this includes a reason.