From 7cfa47d569e8cf89623f6f7078ebbd95067d76a5 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Tue, 22 Sep 2026 15:45:01 -0500 Subject: [PATCH] tweaks to error handling in iputil.IPv6FindUpperProtocol (#1893) --- iputil/checksum.go | 16 +++------------- iputil/packet.go | 31 +++++++++++++------------------ iputil/packet_test.go | 12 ++++++------ outside.go | 2 +- outside_test.go | 14 +++++++------- 5 files changed, 30 insertions(+), 45 deletions(-) diff --git a/iputil/checksum.go b/iputil/checksum.go index 08efb7fd..903a1514 100644 --- a/iputil/checksum.go +++ b/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 - // non-zero offset) does not carry. + // do not attempt to set checksums for fragments 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. - // 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. + // we must find the transport header in order to write a checksum 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 -// section 8.1). +// computed (RFC 768), and over IPv6 the checksum is mandatory (RFC 8200 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) diff --git a/iputil/packet.go b/iputil/packet.go index 88f72dcc..166ede60 100644 --- a/iputil/packet.go +++ b/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 -// offset it begins at, and whether the packet is a non-first fragment. Only the RFC 8200 and IANA extension -// headers below are walked. Everything else, including Mobility (135), HIP (139), Shim6 (140), experimental -// 253/254, and real upper layer protocols like SCTP or GRE, is terminal. Walking those as extension headers -// is a firewall bypass, so they fail closed. 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. 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. +// 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. +// Only the RFC 8200 and IANA extension headers below are walked. +// Everything else, including Mobility (135), HIP (139), Shim6 (140), experimental 253/254, and real upper layer protocols like SCTP or GRE, is terminal. +// 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. +// Returns ErrIPv6CouldNotFindPayload if packet is smaller than an ipv6 header, or the chain is truncated, or if more than 8 extension headers are present. 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 - // closed instead of handing back a mid-chain offset a caller would read ports from. - return nextHeader, offset, isFragment, anyFragment, ErrIPv6CouldNotFindPayload + // Fail closed + return 0, 0, false, false, ErrIPv6CouldNotFindPayload } func CreateICMPEchoResponse(packet, out []byte) []byte { diff --git a/iputil/packet_test.go b/iputil/packet_test.go index 38c5da10..b88a7926 100644 --- a/iputil/packet_test.go +++ b/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) - return + require.ErrorIs(t, err, tt.wantErr) + } 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) diff --git a/outside.go b/outside.go index 8c3ed0a3..c28fe1ea 100644 --- a/outside.go +++ b/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 diff --git a/outside_test.go b/outside_test.go index 6847b887..cf76fc47 100644 --- a/outside_test.go +++ b/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