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

@@ -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 {
+func ipv6CreateRejectICMPPacket(packet []byte, out []byte, proto uint8, offset int) []byte {
-	nextHeader := packet[6]
+	// Do not generate ICMPv6 errors in response to ICMPv6 error packets
-	offset := ipv6.HeaderLen
+	if proto == 58 && len(packet) > offset {
-
+		icmpType := packet[offset]
-	for {
+		if icmpType >= 1 && icmpType <= 4 {
-		switch nextHeader {
+			return nil
-		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) []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 {
+func ipv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragment bool) {
-	nextHeader := packet[6]
+	nextHeader = packet[6]
-	offset := ipv6.HeaderLen
+	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
 		}
 	}
 }

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)