tweaks to error handling in iputil.IPv6FindUpperProtocol (#1893)

iputil/checksum.go

@@ -45,8 +45,7 @@ func setTransportChecksum4(packet []byte) {
 	if ihl < ipv4.HeaderLen || end < ihl || end > len(packet) {
 		return
 	}
-	// The checksum covers the whole datagram, which a fragment (MF set or a
+	// do not attempt to set checksums for fragments
-	// non-zero offset) does not carry.
 	if binary.BigEndian.Uint16(packet[6:8])&0x3fff != 0 {
 		return
 	}
@@ -68,10 +67,7 @@ func setTransportChecksum6(packet []byte) {
 		return
 	}
 
-	// The checksum covers the whole datagram, which a fragment does not carry.
+	// we must find the transport header in order to write a checksum
-	// An unknown extension header hides where the transport header starts. A
-	// chain longer than the walk's budget ends it early, at an offset that was
-	// never checked against the packet.
 	proto, offset, _, anyFragment, err := IPv6FindUpperProtocol(packet[:end])
 	if err != nil || anyFragment || offset >= end {
 		return
@@ -109,8 +105,7 @@ func transportExtent(transport []byte, proto uint8) ([]byte, bool) {
 // writeTransportChecksum stores the checksum of transport, taken over the
 // pseudo-header sum csum, in the header's checksum field. A UDP checksum that
 // computes to zero goes on the wire as 0xffff: zero means no checksum was
-// computed (RFC 768), and over IPv6 the checksum is mandatory (RFC 8200
+// computed (RFC 768), and over IPv6 the checksum is mandatory (RFC 8200 section 8.1).
-// section 8.1).
 func writeTransportChecksum(transport []byte, proto uint8, csum uint32) {
 	var at, minLen int
 	switch proto {
@@ -133,11 +128,6 @@ func writeTransportChecksum(transport []byte, proto uint8, csum uint32) {
 	binary.BigEndian.PutUint16(transport[at:], sum)
 }
 
-// fold reduces a pseudo-header sum to the 16 bit seed Checksum takes. Carrying
-// the high half back into the low half is what keeps the reduction lossless, so
-// the seed sums exactly as the wider value would; 0xffff is its fixed point.
-// Every term of that sum comes from a 16 bit field, so it stays far below the
-// width at which the accumulator would wrap.
 func fold(csum uint32) uint16 {
 	for csum > 0xffff {
 		csum = (csum >> 16) + (csum & 0xffff)

iputil/packet.go

@@ -345,14 +345,11 @@ func ipv6CreateRejectTCPPacket(packet []byte, out []byte, offset int) []byte {
 	return out
 }
 
-// IPv6FindUpperProtocol walks the ipv6 extension header chain and returns the upper layer protocol, the
+// IPv6FindUpperProtocol walks the ipv6 extension header chain and returns the upper layer protocol, the offset it begins at, and whether the packet is a non-first fragment.
-// offset it begins at, and whether the packet is a non-first fragment. Only the RFC 8200 and IANA extension
+// Only the RFC 8200 and IANA extension headers below are walked.
-// headers below are walked. Everything else, including Mobility (135), HIP (139), Shim6 (140), experimental
+// Everything else, including Mobility (135), HIP (139), Shim6 (140), experimental 253/254, and real upper layer protocols like SCTP or GRE, is terminal.
-// 253/254, and real upper layer protocols like SCTP or GRE, is terminal. Walking those as extension headers
+// For a non-first fragment the returned protocol is the fragmented protocol and offset points at the fragment header, there is no transport header to locate.
-// is a firewall bypass, so they fail closed. For a non-first fragment the returned protocol is the fragmented
+// Returns ErrIPv6CouldNotFindPayload if packet is smaller than an ipv6 header, or the chain is truncated, or if more than 8 extension headers are present.
-// protocol and offset points at the fragment header, there is no transport header to locate. Returns
-// ErrIPv6CouldNotFindPayload if packet is smaller than an ipv6 header, or the chain is truncated or longer
-// than the walk's budget before a terminal protocol is reached.
 func IPv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragment bool, anyFragment bool, err error) {
 	const maxIPv6ExtHeaders = 8
 	if len(packet) < ipv6.HeaderLen {
@@ -365,14 +362,14 @@ func IPv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragm
 		switch nextHeader {
 		case 0, 43, 60: // Hop-by-Hop, Routing, Destination
 			if len(packet) < offset+2 {
-				return nextHeader, offset, isFragment, anyFragment, ErrIPv6CouldNotFindPayload
+				return 0, 0, false, false, ErrIPv6CouldNotFindPayload
 			}
 			nextHeader = packet[offset]
 			offset += (int(packet[offset+1]) + 1) << 3
 
 		case 44: // Fragment
 			if len(packet) < offset+8 {
-				return nextHeader, offset, isFragment, anyFragment, ErrIPv6CouldNotFindPayload
+				return 0, 0, false, false, ErrIPv6CouldNotFindPayload
 			}
 			anyFragment = true
 			// Non-first fragments carry no transport header, report the fragmented protocol and stop
@@ -384,23 +381,21 @@ func IPv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragm
 
 		case 51: // AH
 			if len(packet) < offset+2 {
-				return nextHeader, offset, isFragment, anyFragment, ErrIPv6CouldNotFindPayload
+				return 0, 0, false, false, ErrIPv6CouldNotFindPayload
 			}
 			nextHeader = packet[offset]
 			offset += (int(packet[offset+1]) + 2) << 2
 
 		default:
-			// A prior extension header can declare a length that advances offset past the packet. The terminal
-			// protocol's header isn't actually here, so treat the chain as truncated rather than classifying it.
 			if offset > len(packet) {
-				return nextHeader, offset, isFragment, anyFragment, ErrIPv6CouldNotFindPayload
+				// A prior extension header can declare a length that advances offset past the packet, reject this.
+				return 0, 0, false, false, ErrIPv6CouldNotFindPayload
 			}
-			return nextHeader, offset, isFragment, anyFragment, nil
+			return nextHeader, offset, isFragment, anyFragment, nil //success!
 		}
 	}
-	// Budget spent without reaching a terminal protocol. We don't know where the transport starts, so fail
+	// Fail closed
-	// closed instead of handing back a mid-chain offset a caller would read ports from.
+	return 0, 0, false, false, ErrIPv6CouldNotFindPayload
-	return nextHeader, offset, isFragment, anyFragment, ErrIPv6CouldNotFindPayload
 }
 
 func CreateICMPEchoResponse(packet, out []byte) []byte {

iputil/packet_test.go

@@ -535,7 +535,7 @@ func Test_IPv6FindUpperProtocol(t *testing.T) {
 	// report 4040546 shape: without failing closed the walk hands back offset 104, and a caller reads ports
 	// from the attacker's bytes there.
 	var budgetChain []byte
-	for i := 0; i < 7; i++ {
+	for range 7 {
 		budgetChain = append(budgetChain, extToHbH...)
 	}
 	budgetChain = append(budgetChain, extToTCP...)
@@ -560,9 +560,9 @@ func Test_IPv6FindUpperProtocol(t *testing.T) {
 		{"first fragment walks to transport", 44, append(firstFragToUDP, transport...), 17, ipv6.HeaderLen + 8, false, true, nil},
 		{"non-first fragment stops", 44, append(nonFirstFrag, transport...), 17, ipv6.HeaderLen, true, true, nil},
 		{"unknown protocol is terminal", 132, transport, 132, ipv6.HeaderLen, false, false, nil}, // SCTP
-		{"truncated extension header", 0, nil, 0, ipv6.HeaderLen, false, false, ErrIPv6CouldNotFindPayload},
+		{"truncated extension header", 0, nil, 0, 0, false, false, ErrIPv6CouldNotFindPayload},
 		// Destination Options with a declared length (255+1)*8 = 2048 that runs past the 48 byte buffer, next = SCTP
-		{"extension length past buffer", 60, []byte{132, 255, 0, 0, 0, 0, 0, 0}, 132, ipv6.HeaderLen + 2048, false, false, ErrIPv6CouldNotFindPayload},
+		{"extension length past buffer", 0, []byte{132, 255, 0, 0, 0, 0, 0, 0}, 0, 0, false, false, ErrIPv6CouldNotFindPayload},
 		{"budget exhausted fails closed", 0, budgetChain, 0, 0, false, false, ErrIPv6CouldNotFindPayload},
 	}
 
@@ -571,10 +571,10 @@ func Test_IPv6FindUpperProtocol(t *testing.T) {
 			packet := makeIPv6Packet(src, dst, tt.nextHeader, tt.payload)
 			proto, offset, isFragment, anyFragment, err := IPv6FindUpperProtocol(packet)
 			if tt.wantErr != nil {
-				assert.ErrorIs(t, err, tt.wantErr)
+				require.ErrorIs(t, err, tt.wantErr)
-				return
+			} else {
+				require.NoError(t, err)
 			}
-			require.NoError(t, err)
 			assert.Equal(t, tt.wantProto, proto)
 			assert.Equal(t, tt.wantOffset, offset)
 			assert.Equal(t, tt.wantFragment, isFragment)

outside.go

@@ -354,7 +354,7 @@ func parseV6(data []byte, incoming bool, fp *firewall.ParsedPacket) error {
 	// and cannot drift into misreading an unknown protocol (SCTP, GRE, etc.) as a forged transport.
 	proto, offset, isFragment, anyFragment, err := iputil.IPv6FindUpperProtocol(data)
 	if err != nil {
-		return ErrIPv6PacketTooShort
+		return err
 	}
 
 	fp.Protocol = proto

outside_test.go

@@ -117,7 +117,7 @@ func Test_newPacket_v6(t *testing.T) {
 	require.NoError(t, err)
 
 	err = newPacket(buffer.Bytes(), true, p)
-	require.ErrorIs(t, err, ErrIPv6PacketTooShort)
+	require.ErrorIs(t, err, iputil.ErrIPv6CouldNotFindPayload)
 
 	// A v6 packet with a hop-by-hop extension
 	// ICMPv6 Payload (Echo Request)
@@ -151,12 +151,12 @@ func Test_newPacket_v6(t *testing.T) {
 	// A full IPv6 header and 1 byte in the first extension, but missing
 	// the length byte.
 	err = newPacket(buffer.Bytes()[:41], true, p)
-	require.ErrorIs(t, err, ErrIPv6PacketTooShort)
+	require.ErrorIs(t, err, iputil.ErrIPv6CouldNotFindPayload)
 
 	// A full IPv6 header plus 1 full extension, but only 1 byte of the
 	// next layer, missing length byte
 	err = newPacket(buffer.Bytes()[:49], true, p)
-	require.ErrorIs(t, err, ErrIPv6PacketTooShort)
+	require.ErrorIs(t, err, iputil.ErrIPv6CouldNotFindPayload)
 	err = nil
 
 	// A good ICMP packet
@@ -354,12 +354,12 @@ func Test_newPacket_v6(t *testing.T) {
 
 	// Ensure buffer bounds checking during processing, a truncated AH header can't reach the payload
 	err = newPacket(b[:41], true, p)
-	require.ErrorIs(t, err, ErrIPv6PacketTooShort)
+	require.ErrorIs(t, err, iputil.ErrIPv6CouldNotFindPayload)
 
 	// Invalid AH header
 	b = buffer.Bytes()
 	err = newPacket(b, true, p)
-	require.ErrorIs(t, err, ErrIPv6PacketTooShort)
+	require.ErrorIs(t, err, iputil.ErrIPv6CouldNotFindPayload)
 }
 
 func Test_newPacket_ipv6Fragment(t *testing.T) {
@@ -466,7 +466,7 @@ func Test_newPacket_ipv6Fragment(t *testing.T) {
 
 	// Too short of a fragment packet
 	err = newPacket(secondFrag[:len(secondFrag)-10], false, p)
-	require.ErrorIs(t, err, ErrIPv6PacketTooShort)
+	require.ErrorIs(t, err, iputil.ErrIPv6CouldNotFindPayload)
 }
 
 func BenchmarkParseV6(b *testing.B) {
@@ -707,7 +707,7 @@ func Test_newPacket_v6ExtHeaderPastBuffer(t *testing.T) {
 	pkt[40] = byte(layers.IPProtocolSCTP)           // Dest Options next header = SCTP
 	pkt[41] = 255                                   // declared length (255+1)*8 = 2048, past the 48 byte buffer
 
-	require.ErrorIs(t, newPacket(pkt, true, p), ErrIPv6PacketTooShort)
+	require.ErrorIs(t, newPacket(pkt, true, p), iputil.ErrIPv6CouldNotFindPayload)
 }
 
 // Test_newPacket_v6ExtHeaderConfusion is a regression test for parseV6 walking any unrecognized