From 7d3166a19df7d1f308d9021d41ce18b4fe9db82b Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Tue, 16 Jun 2026 16:51:14 -0400 Subject: [PATCH] cleanup ipv6 iputil helpers / skip reject for ICMP error packets and fragments (#1768) * cleanup ipv6 iputil helpers With my refactoring in this PR I accidentally had some duplicate logic, this PR cleans it up: - https://github.com/slackhq/nebula/pull/1766 * skip ICMP reject for ICMP error packets and fragments Per RFC 1122, ICMP error messages must not be generated in response to other ICMP error messages to prevent infinite error loops. This applies to both IPv4 (types 3, 4, 5, 11, 12) and IPv6 (types 1-4). Do not generate reject packets for IPv4 or IPv6 fragments. For IPv4, check MF flag and fragment offset. For IPv6, add isFragment return to ipv6FindUpperProtocol so a single traversal handles both protocol lookup and fragment detection. * do send rejects for the initial fragment RFC says "non-initial fragment"s * fix fragment checks --- iputil/packet.go | 78 +++++++++++-------------- iputil/packet_test.go | 132 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 43 deletions(-) diff --git a/iputil/packet.go b/iputil/packet.go index 1cf895ce..99893822 100644 --- a/iputil/packet.go +++ b/iputil/packet.go @@ -35,6 +35,10 @@ func CreateRejectPacket(packet []byte, out []byte) []byte { if len(packet) < ipv4.HeaderLen { return nil } + // Do not send reject packets for non-first fragments + if packet[6]&0x1f != 0 || packet[7] != 0 { + return nil + } switch packet[9] { case 6: // tcp return ipv4CreateRejectTCPPacket(packet, out) @@ -59,6 +63,14 @@ func ipv4CreateRejectICMPPacket(packet []byte, out []byte) []byte { return nil } + // Do not generate ICMP errors in response to ICMP error packets + if packet[9] == 1 && len(packet) > ihl { + icmpType := packet[ihl] + if icmpType == 3 || icmpType == 4 || icmpType == 5 || icmpType == 11 || icmpType == 12 { + return nil + } + } + // ICMP reply includes original header and first 8 bytes of the packet packetLen := min(len(packet), ihl+8) @@ -187,49 +199,27 @@ func ipv4CreateRejectTCPPacket(packet []byte, out []byte) []byte { } func ipv6CreateRejectPacket(packet []byte, out []byte) []byte { - proto := ipv6FindUpperProtocol(packet) + proto, offset, isFragment := ipv6FindUpperProtocol(packet) + if isFragment { + return nil + } switch proto { case 6: // tcp - return ipv6CreateRejectTCPPacket(packet, out) + return ipv6CreateRejectTCPPacket(packet, out, offset) default: - return ipv6CreateRejectICMPPacket(packet, out) + return ipv6CreateRejectICMPPacket(packet, out, proto, offset) } } -func ipv6FindUpperProtocol(packet []byte) uint8 { - nextHeader := packet[6] - offset := ipv6.HeaderLen - - for { - switch nextHeader { - case 0, 43, 60: // Hop-by-Hop, Routing, Destination - if len(packet) < offset+2 { - return nextHeader - } - nextHeader = packet[offset] - offset += int(packet[offset+1]+1) << 3 - - case 44: // Fragment - if len(packet) < offset+8 { - return nextHeader - } - nextHeader = packet[offset] - offset += 8 - - case 51: // AH - if len(packet) < offset+2 { - return nextHeader - } - nextHeader = packet[offset] - offset += int(packet[offset+1]+2) << 2 - - default: - return nextHeader +func ipv6CreateRejectICMPPacket(packet []byte, out []byte, proto uint8, offset int) []byte { + // Do not generate ICMPv6 errors in response to ICMPv6 error packets + if proto == 58 && len(packet) > offset { + icmpType := packet[offset] + if icmpType >= 1 && icmpType <= 4 { + return nil } } -} -func ipv6CreateRejectICMPPacket(packet []byte, out []byte) []byte { // Include as much of the original packet as possible, up to 1000 bytes, // so the response fits comfortably within any tunnel MTU. packetLen := min(len(packet), 1000) @@ -277,10 +267,9 @@ func ipv6CreateRejectICMPPacket(packet []byte, out []byte) []byte { return out } -func ipv6CreateRejectTCPPacket(packet []byte, out []byte) []byte { +func ipv6CreateRejectTCPPacket(packet []byte, out []byte, offset int) []byte { const tcpLen = 20 - offset := ipv6FindUpperProtocolOffset(packet) if len(packet) < offset+tcpLen { return nil } @@ -344,35 +333,38 @@ func ipv6CreateRejectTCPPacket(packet []byte, out []byte) []byte { return out } -func ipv6FindUpperProtocolOffset(packet []byte) int { - nextHeader := packet[6] - offset := ipv6.HeaderLen +func ipv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragment bool) { + nextHeader = packet[6] + offset = ipv6.HeaderLen for { switch nextHeader { case 0, 43, 60: // Hop-by-Hop, Routing, Destination if len(packet) < offset+2 { - return offset + return nextHeader, offset, isFragment } nextHeader = packet[offset] offset += int(packet[offset+1]+1) << 3 case 44: // Fragment if len(packet) < offset+8 { - return offset + return nextHeader, offset, isFragment + } + if packet[offset+2] != 0 || packet[offset+3]&0xf8 != 0 { + isFragment = true } nextHeader = packet[offset] offset += 8 case 51: // AH if len(packet) < offset+2 { - return offset + return nextHeader, offset, isFragment } nextHeader = packet[offset] offset += int(packet[offset+1]+2) << 2 default: - return offset + return nextHeader, offset, isFragment } } } diff --git a/iputil/packet_test.go b/iputil/packet_test.go index 346d4dd3..6d567d51 100644 --- a/iputil/packet_test.go +++ b/iputil/packet_test.go @@ -74,6 +74,111 @@ func Test_CreateRejectPacket(t *testing.T) { assert.Len(t, rejectPacket, expectedLen) } +func Test_CreateRejectPacket_NoFragment(t *testing.T) { + out := make([]byte, MaxRejectPacketSize) + + // IPv4: non-zero fragment offset should not generate reject packet + h := ipv4.Header{ + Len: 20, + Src: net.IPv4(10, 0, 0, 1), + Dst: net.IPv4(10, 0, 0, 2), + Protocol: 17, // UDP + } + b, err := h.Marshal() + if err != nil { + t.Fatalf("h.Marshal: %v", err) + } + b = append(b, make([]byte, 8)...) + // Set fragment offset to non-zero (byte 6-7, offset in 8-byte units) + b[6] = 0x00 + b[7] = 0x01 + assert.Nil(t, CreateRejectPacket(b, out)) + + // MF flag with zero offset (first fragment) should still generate reject + b[6] = 0x20 // MF flag set + b[7] = 0x00 + assert.NotNil(t, CreateRejectPacket(b, out)) + + // Non-fragment should still generate reject packet + b[6] = 0x00 + b[7] = 0x00 + assert.NotNil(t, CreateRejectPacket(b, out)) + + // DF flag only (not a fragment) should still generate reject packet + b[6] = 0x40 + b[7] = 0x00 + assert.NotNil(t, CreateRejectPacket(b, out)) +} + +func Test_CreateRejectPacketIPv6_NoFragment(t *testing.T) { + src := net.ParseIP("fd00::1") + dst := net.ParseIP("fd00::2") + out := make([]byte, MaxRejectPacketSize) + + // IPv6 with Fragment header and non-zero offset should not generate reject + fragHeader := []byte{ + 17, // next header: UDP + 0, // reserved + 0, 9, // fragment offset=1 (shifted left 3), M=1 + 0, 0, 0, 1, // identification + } + udpPayload := make([]byte, 8) + payload := append(fragHeader, udpPayload...) + packet := makeIPv6Packet(src, dst, 44, payload) // next header 44 = Fragment + assert.Nil(t, CreateRejectPacket(packet, out)) + + // Fragment header with zero offset (first fragment) should still generate reject + fragHeader[2] = 0 + fragHeader[3] = 1 // offset=0, M=1 + payload = append(fragHeader, udpPayload...) + packet = makeIPv6Packet(src, dst, 44, payload) + assert.NotNil(t, CreateRejectPacket(packet, out)) +} + +func Test_CreateRejectPacket_NoICMPError(t *testing.T) { + out := make([]byte, MaxRejectPacketSize) + + // ICMP error types should not generate reject packets + icmpErrorTypes := []byte{3, 4, 5, 11, 12} + for _, icmpType := range icmpErrorTypes { + h := ipv4.Header{ + Len: 20, + Src: net.IPv4(10, 0, 0, 1), + Dst: net.IPv4(10, 0, 0, 2), + Protocol: 1, // ICMP + } + + b, err := h.Marshal() + if err != nil { + t.Fatalf("h.Marshal: %v", err) + } + b = append(b, icmpType, 0, 0, 0, 0, 0, 0, 0) + + rejectPacket := CreateRejectPacket(b, out) + assert.Nil(t, rejectPacket, "ICMP type %d should not generate a reject packet", icmpType) + } + + // ICMP non-error types should still generate reject packets + icmpNonErrorTypes := []byte{0, 8, 13, 14} + for _, icmpType := range icmpNonErrorTypes { + h := ipv4.Header{ + Len: 20, + Src: net.IPv4(10, 0, 0, 1), + Dst: net.IPv4(10, 0, 0, 2), + Protocol: 1, // ICMP + } + + b, err := h.Marshal() + if err != nil { + t.Fatalf("h.Marshal: %v", err) + } + b = append(b, icmpType, 0, 0, 0, 0, 0, 0, 0) + + rejectPacket := CreateRejectPacket(b, out) + assert.NotNil(t, rejectPacket, "ICMP type %d should generate a reject packet", icmpType) + } +} + func makeIPv6Packet(src, dst net.IP, nextHeader uint8, payload []byte) []byte { b := make([]byte, ipv6.HeaderLen+len(payload)) b[0] = ipv6.Version << 4 @@ -197,6 +302,33 @@ func Test_CreateRejectPacketIPv6_TCPWithACK(t *testing.T) { assert.Equal(t, uint32(2000), binary.BigEndian.Uint32(tcpOut[4:])) } +func Test_CreateRejectPacketIPv6_NoICMPError(t *testing.T) { + src := net.ParseIP("fd00::1") + dst := net.ParseIP("fd00::2") + out := make([]byte, MaxRejectPacketSize) + + // ICMPv6 error types (1-4) should not generate reject packets + for icmpType := byte(1); icmpType <= 4; icmpType++ { + payload := make([]byte, 8) + payload[0] = icmpType + packet := makeIPv6Packet(src, dst, 58, payload) + + rejectPacket := CreateRejectPacket(packet, out) + assert.Nil(t, rejectPacket, "ICMPv6 type %d should not generate a reject packet", icmpType) + } + + // ICMPv6 non-error types should still generate reject packets + nonErrorTypes := []byte{128, 129, 133, 134} + for _, icmpType := range nonErrorTypes { + payload := make([]byte, 8) + payload[0] = icmpType + packet := makeIPv6Packet(src, dst, 58, payload) + + rejectPacket := CreateRejectPacket(packet, out) + assert.NotNil(t, rejectPacket, "ICMPv6 type %d should generate a reject packet", icmpType) + } +} + func Test_CreateRejectPacketIPv6_TooShort(t *testing.T) { // Packet too short to be valid IPv6 out := make([]byte, MaxRejectPacketSize)