diff --git a/CHANGELOG.md b/CHANGELOG.md index b86444f1..fb17eca5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- IPv6 packets whose next header is a protocol Nebula does not parse (SCTP, GRE, IP-in-IP, etc.) are now + classified as that protocol with no ports, closing a firewall bypass where a crafted payload could steer + the classifier into reading one as TCP/UDP and matching a TCP/UDP rule. These packets are now matched as + their true protocol, so only a `proto: any` rule allows them. If you carry one of these protocols over the + overlay, confirm a `proto: any` rule covers it before upgrading, it may have been passing only through this + bypass. (#1840) + +### Fixed + +- The ICMPv6 type was read from the wrong byte when classifying IPv6 packets, so the echo identifier used + for conntrack was never picked up. (#1840) + ## [1.11.0] - 2026-07-23 See the [v1.11.0](https://github.com/slackhq/nebula/milestone/25?closed=1) milestone for a complete list of changes. diff --git a/iputil/packet.go b/iputil/packet.go index c0c1921e..f91988e2 100644 --- a/iputil/packet.go +++ b/iputil/packet.go @@ -2,11 +2,16 @@ package iputil import ( "encoding/binary" + "errors" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" ) +// ErrIPv6CouldNotFindPayload is returned when the ipv6 extension header chain is truncated before a terminal +// upper layer protocol is reached. +var ErrIPv6CouldNotFindPayload = errors.New("could not find payload in ipv6 packet") + const ( // MaxIPv4RejectPacketSize is the largest IPv4 reject packet: // - 20 byte ipv4 header @@ -199,8 +204,8 @@ func ipv4CreateRejectTCPPacket(packet []byte, out []byte) []byte { } func ipv6CreateRejectPacket(packet []byte, out []byte) []byte { - proto, offset, isFragment := ipv6FindUpperProtocol(packet) - if isFragment { + proto, offset, isFragment, err := IPv6FindUpperProtocol(packet) + if err != nil || isFragment { return nil } switch proto { @@ -333,7 +338,18 @@ func ipv6CreateRejectTCPPacket(packet []byte, out []byte, offset int) []byte { return out } -func ipv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragment bool) { +// 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 before a +// terminal protocol is reached. +func IPv6FindUpperProtocol(packet []byte) (nextHeader uint8, offset int, isFragment bool, err error) { + if len(packet) < ipv6.HeaderLen { + return 0, 0, false, ErrIPv6CouldNotFindPayload + } nextHeader = packet[6] offset = ipv6.HeaderLen @@ -341,30 +357,36 @@ 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 + return nextHeader, offset, isFragment, ErrIPv6CouldNotFindPayload } nextHeader = packet[offset] offset += (int(packet[offset+1]) + 1) << 3 case 44: // Fragment if len(packet) < offset+8 { - return nextHeader, offset, isFragment + return nextHeader, offset, isFragment, ErrIPv6CouldNotFindPayload } + // Non-first fragments carry no transport header, report the fragmented protocol and stop if packet[offset+2] != 0 || packet[offset+3]&0xf8 != 0 { - isFragment = true + return packet[offset], offset, true, nil } nextHeader = packet[offset] offset += 8 case 51: // AH if len(packet) < offset+2 { - return nextHeader, offset, isFragment + return nextHeader, offset, isFragment, ErrIPv6CouldNotFindPayload } nextHeader = packet[offset] offset += (int(packet[offset+1]) + 2) << 2 default: - return nextHeader, offset, isFragment + // 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, ErrIPv6CouldNotFindPayload + } + return nextHeader, offset, isFragment, nil } } } diff --git a/iputil/packet_test.go b/iputil/packet_test.go index 6d567d51..00de5382 100644 --- a/iputil/packet_test.go +++ b/iputil/packet_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" ) @@ -474,3 +475,61 @@ func TestCreateICMPEchoResponse_IPv6_NotICMPv6(t *testing.T) { result := CreateICMPEchoResponse(packet, out) assert.Nil(t, result) } + +func Test_IPv6FindUpperProtocol(t *testing.T) { + src := net.ParseIP("fd00::1") + dst := net.ParseIP("fd00::2") + + // 8 byte extension/transport stand-ins, first byte is the next header, second is the length field + extToTCP := []byte{6, 0, 0, 0, 0, 0, 0, 0} // len 0 -> 8 bytes, next = TCP + extToUDP := []byte{17, 0, 0, 0, 0, 0, 0, 0} // len 0 -> 8 bytes, next = UDP + extToRouting := []byte{43, 0, 0, 0, 0, 0, 0, 0} // len 0 -> 8 bytes, next = Routing + ahToUDP := []byte{17, 0, 0, 0, 0, 0, 0, 0} // AH len 0 -> (0+2)<<2 = 8 bytes, next = UDP + firstFragToUDP := []byte{17, 0, 0, 1, 0, 0, 0, 1} // frag offset 0, M=1, next = UDP + nonFirstFrag := []byte{17, 0, 0, 9, 0, 0, 0, 1} // frag offset non-zero, next = UDP + transport := []byte{0, 80, 1, 187, 0, 0, 0, 0} // stand-in bytes, IPv6FindUpperProtocol never reads ports + + tests := []struct { + name string + nextHeader uint8 + payload []byte + wantProto uint8 + wantOffset int + wantFragment bool + wantErr error + }{ + {"plain udp", 17, transport, 17, ipv6.HeaderLen, false, nil}, + {"hop-by-hop then tcp", 0, append(extToTCP, transport...), 6, ipv6.HeaderLen + 8, false, nil}, + {"routing then tcp", 43, append(extToTCP, transport...), 6, ipv6.HeaderLen + 8, false, nil}, + {"destination then udp", 60, append(extToUDP, transport...), 17, ipv6.HeaderLen + 8, false, nil}, + {"hop-by-hop, routing, then tcp", 0, append(append(extToRouting, extToTCP...), transport...), 6, ipv6.HeaderLen + 16, false, nil}, + {"ah then udp", 51, append(ahToUDP, transport...), 17, ipv6.HeaderLen + 8, false, nil}, + {"first fragment walks to transport", 44, append(firstFragToUDP, transport...), 17, ipv6.HeaderLen + 8, false, nil}, + {"non-first fragment stops", 44, append(nonFirstFrag, transport...), 17, ipv6.HeaderLen, true, nil}, + {"unknown protocol is terminal", 132, transport, 132, ipv6.HeaderLen, false, nil}, // SCTP + {"truncated extension header", 0, nil, 0, ipv6.HeaderLen, 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, ErrIPv6CouldNotFindPayload}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + packet := makeIPv6Packet(src, dst, tt.nextHeader, tt.payload) + proto, offset, isFragment, err := IPv6FindUpperProtocol(packet) + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantProto, proto) + assert.Equal(t, tt.wantOffset, offset) + assert.Equal(t, tt.wantFragment, isFragment) + }) + } + + // A packet smaller than an ipv6 header must error rather than panic reading byte 6 + t.Run("shorter than ipv6 header", func(t *testing.T) { + _, _, _, err := IPv6FindUpperProtocol(make([]byte, 6)) + assert.ErrorIs(t, err, ErrIPv6CouldNotFindPayload) + }) +} diff --git a/outside.go b/outside.go index cf56bb4a..b135110e 100644 --- a/outside.go +++ b/outside.go @@ -13,6 +13,7 @@ import ( "github.com/slackhq/nebula/firewall" "github.com/slackhq/nebula/header" + "github.com/slackhq/nebula/iputil" "golang.org/x/net/ipv4" ) @@ -299,7 +300,6 @@ var ( ErrIPv4InvalidHeaderLength = errors.New("invalid ipv4 header length") ErrIPv4PacketTooShort = errors.New("ipv4 packet is too short") ErrIPv6PacketTooShort = errors.New("ipv6 packet is too short") - ErrIPv6CouldNotFindPayload = errors.New("could not find payload in ipv6 packet") ) // newPacket validates and parses the interesting bits for the firewall out of the ip and sub protocol headers @@ -332,101 +332,59 @@ func parseV6(data []byte, incoming bool, fp *firewall.Packet) error { fp.RemoteAddr, _ = netip.AddrFromSlice(data[24:40]) } - protoAt := 6 // NextHeader is at 6 bytes into the ipv6 header - offset := ipv6.HeaderLen // Start at the end of the ipv6 header - next := 0 - for { - if protoAt >= dataLen { - break + // Walk the extension header chain to the upper layer protocol. iputil.IPv6FindUpperProtocol is the single + // source of truth for which headers are extension headers, so this stays in lockstep with the reject path + // and cannot drift into misreading an unknown protocol (SCTP, GRE, etc.) as a forged transport. + proto, offset, isFragment, err := iputil.IPv6FindUpperProtocol(data) + if err != nil { + return ErrIPv6PacketTooShort + } + + fp.Protocol = proto + fp.Fragment = isFragment + if isFragment { + // Non-first fragments carry no transport header, so we have no ports to read + fp.RemotePort = 0 + fp.LocalPort = 0 + return nil + } + + switch layers.IPProtocol(proto) { + case layers.IPProtocolICMPv6: + // An ICMPv6 message is at least type, code and checksum, 4 bytes. Only echo carries more than we read. + if dataLen < offset+4 { + return ErrIPv6PacketTooShort } - proto := layers.IPProtocol(data[protoAt]) - - switch proto { - case layers.IPProtocolESP, layers.IPProtocolNoNextHeader: - fp.Protocol = uint8(proto) - fp.RemotePort = 0 - fp.LocalPort = 0 - fp.Fragment = false - return nil - - case layers.IPProtocolICMPv6: + fp.LocalPort = 0 //incoming vs outgoing doesn't matter for icmpv6 + switch data[offset] { //icmp type + case layers.ICMPv6TypeEchoRequest, layers.ICMPv6TypeEchoReply: if dataLen < offset+6 { return ErrIPv6PacketTooShort } - fp.Protocol = uint8(proto) - fp.LocalPort = 0 //incoming vs outgoing doesn't matter for icmpv6 - icmptype := data[offset+1] - switch icmptype { - case layers.ICMPv6TypeEchoRequest, layers.ICMPv6TypeEchoReply: - fp.RemotePort = binary.BigEndian.Uint16(data[offset+4 : offset+6]) //identifier - default: - fp.RemotePort = 0 - } - fp.Fragment = false - return nil - - case layers.IPProtocolTCP, layers.IPProtocolUDP: - if dataLen < offset+4 { - return ErrIPv6PacketTooShort - } - - fp.Protocol = uint8(proto) - if incoming { - fp.RemotePort = binary.BigEndian.Uint16(data[offset : offset+2]) - fp.LocalPort = binary.BigEndian.Uint16(data[offset+2 : offset+4]) - } else { - fp.LocalPort = binary.BigEndian.Uint16(data[offset : offset+2]) - fp.RemotePort = binary.BigEndian.Uint16(data[offset+2 : offset+4]) - } - - fp.Fragment = false - return nil - - case layers.IPProtocolIPv6Fragment: - // Fragment header is 8 bytes, need at least offset+4 to read the offset field - if dataLen < offset+8 { - return ErrIPv6PacketTooShort - } - - // Check if this is the first fragment - fragmentOffset := binary.BigEndian.Uint16(data[offset+2:offset+4]) &^ uint16(0x7) // Remove the reserved and M flag bits - if fragmentOffset != 0 { - // Non-first fragment, use what we have now and stop processing - fp.Protocol = data[offset] - fp.Fragment = true - fp.RemotePort = 0 - fp.LocalPort = 0 - return nil - } - - // The next loop should be the transport layer since we are the first fragment - next = 8 // Fragment headers are always 8 bytes - - case layers.IPProtocolAH: - // Auth headers, used by IPSec, have a different meaning for header length - if dataLen <= offset+1 { - break - } - next = (int(data[offset+1]) + 2) << 2 - + fp.RemotePort = binary.BigEndian.Uint16(data[offset+4 : offset+6]) //identifier default: - // Normal ipv6 header length processing - if dataLen <= offset+1 { - break - } - next = (int(data[offset+1]) + 1) << 3 + fp.RemotePort = 0 } - if next <= 0 { - // Safety check, each ipv6 header has to be at least 8 bytes - next = 8 + case layers.IPProtocolTCP, layers.IPProtocolUDP: + if dataLen < offset+4 { + return ErrIPv6PacketTooShort + } + if incoming { + fp.RemotePort = binary.BigEndian.Uint16(data[offset : offset+2]) + fp.LocalPort = binary.BigEndian.Uint16(data[offset+2 : offset+4]) + } else { + fp.LocalPort = binary.BigEndian.Uint16(data[offset : offset+2]) + fp.RemotePort = binary.BigEndian.Uint16(data[offset+2 : offset+4]) } - protoAt = offset - offset = offset + next + default: + // don't set ports for protocols Nebula doesn't inspect + fp.RemotePort = 0 + fp.LocalPort = 0 } - return ErrIPv6CouldNotFindPayload + return nil } func parseV4(data []byte, incoming bool, fp *firewall.Packet) error { diff --git a/outside_test.go b/outside_test.go index 4a24cae5..ebed4564 100644 --- a/outside_test.go +++ b/outside_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/ipv4" + "golang.org/x/net/ipv6" ) func Test_newPacket(t *testing.T) { @@ -115,12 +116,12 @@ func Test_newPacket_v6(t *testing.T) { require.NoError(t, err) err = newPacket(buffer.Bytes(), true, p) - require.ErrorIs(t, err, ErrIPv6CouldNotFindPayload) + require.ErrorIs(t, err, ErrIPv6PacketTooShort) // A v6 packet with a hop-by-hop extension // ICMPv6 Payload (Echo Request) icmpLayer := layers.ICMPv6{ - TypeCode: layers.ICMPv6TypeEchoRequest, + TypeCode: layers.CreateICMPv6TypeCode(layers.ICMPv6TypeEchoRequest, 0), } // Hop-by-Hop Extension Header hopOption := layers.IPv6HopByHopOption{} @@ -149,12 +150,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, ErrIPv6CouldNotFindPayload) + require.ErrorIs(t, err, ErrIPv6PacketTooShort) // 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, ErrIPv6CouldNotFindPayload) + require.ErrorIs(t, err, ErrIPv6PacketTooShort) err = nil // A good ICMP packet @@ -167,7 +168,7 @@ func Test_newPacket_v6(t *testing.T) { } icmp := layers.ICMPv6{ - TypeCode: layers.ICMPv6TypeEchoRequest, + TypeCode: layers.CreateICMPv6TypeCode(layers.ICMPv6TypeEchoRequest, 0), Checksum: 0x1234, } @@ -189,6 +190,18 @@ func Test_newPacket_v6(t *testing.T) { assert.Equal(t, uint16(0), p.LocalPort) assert.False(t, p.Fragment) + // A minimal 4 byte non-echo ICMPv6 message (type, code, checksum), no identifier to read + icmpMin := make([]byte, ipv6.HeaderLen+4) + copy(icmpMin, buffer.Bytes()[:ipv6.HeaderLen]) + icmpMin[6] = byte(layers.IPProtocolICMPv6) + icmpMin[ipv6.HeaderLen] = 1 // type 1, destination unreachable, not echo + err = newPacket(icmpMin, true, p) + require.NoError(t, err) + assert.Equal(t, uint8(layers.IPProtocolICMPv6), p.Protocol) + assert.Equal(t, uint16(0), p.RemotePort) + assert.Equal(t, uint16(0), p.LocalPort) + assert.False(t, p.Fragment) + // A good ESP packet b := buffer.Bytes() b[6] = byte(layers.IPProtocolESP) @@ -213,11 +226,15 @@ func Test_newPacket_v6(t *testing.T) { assert.Equal(t, uint16(0), p.LocalPort) assert.False(t, p.Fragment) - // An unknown protocol packet + // An unknown protocol packet, we don't dissect it so we fail closed on its true protocol with no ports b = buffer.Bytes() b[6] = 255 // 255 is a reserved protocol number err = newPacket(b, true, p) - require.ErrorIs(t, err, ErrIPv6CouldNotFindPayload) + require.NoError(t, err) + assert.Equal(t, uint8(255), p.Protocol) + assert.Equal(t, uint16(0), p.RemotePort) + assert.Equal(t, uint16(0), p.LocalPort) + assert.False(t, p.Fragment) // A good UDP packet ip = layers.IPv6{ @@ -334,14 +351,14 @@ func Test_newPacket_v6(t *testing.T) { assert.Equal(t, uint16(22), p.LocalPort) assert.False(t, p.Fragment) - // Ensure buffer bounds checking during processing + // 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) // Invalid AH header b = buffer.Bytes() err = newPacket(b, true, p) - require.ErrorIs(t, err, ErrIPv6CouldNotFindPayload) + require.ErrorIs(t, err, ErrIPv6PacketTooShort) } func Test_newPacket_ipv6Fragment(t *testing.T) { @@ -675,3 +692,66 @@ func Test_newPacket_v6ExtHeaderOverflow(t *testing.T) { // the host delivers to, not the forged 443 at the overflowed offset. assert.Equal(t, uint16(22), p.LocalPort, "firewall must parse the real transport header, not the overflowed offset") } + +// Test_newPacket_v6ExtHeaderPastBuffer is a regression test for an extension header whose declared length +// advances the walk past the end of the packet. The upper layer protocol's header isn't actually present, +// so parseV6 must drop the packet rather than classify it as the terminal protocol with no ports. +func Test_newPacket_v6ExtHeaderPastBuffer(t *testing.T) { + p := &firewall.Packet{} + + pkt := make([]byte, 48) + pkt[0] = 0x60 + pkt[6] = byte(layers.IPProtocolIPv6Destination) // Destination Options + pkt[7] = 64 // hop limit + 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) +} + +// Test_newPacket_v6ExtHeaderConfusion is a regression test for parseV6 walking any unrecognized +// Next Header as if it were an ipv6 extension header. A real upper layer protocol Nebula doesn't +// dissect (SCTP here) is not walkable, so applying the (len+1)*8 formula marched into the SCTP +// payload and landed on a byte that looked like UDP, forging a protocol/port pair the firewall +// would trust while the host delivered the real SCTP datagram. The fix fails closed: the packet +// is classified as its true protocol with no ports, so it only matches an `any` rule. +func Test_newPacket_v6ExtHeaderConfusion(t *testing.T) { + p := &firewall.Packet{} + + pkt := make([]byte, 52) + pkt[0] = 0x60 // version 6 + pkt[6] = byte(layers.IPProtocolSCTP) // NextHeader = SCTP, a real protocol, not an extension header + pkt[7] = 64 // hop limit + + // Real SCTP header at offset 40. Pre-fix parseV6 walked SCTP as an extension header: byte 41 (0x00, the + // low byte of the src port below) was read as the header length, giving next=(0+1)*8=8, which landed the + // walk on byte 40 (0x11), misread as NextHeader=UDP, then bytes 48-51 as ports. + binary.BigEndian.PutUint16(pkt[40:42], 0x1100) // SCTP src port; byte 40=0x11, byte 41=0x00 + binary.BigEndian.PutUint16(pkt[42:44], 445) // SCTP dst port, never read by parseV6 + binary.BigEndian.PutUint16(pkt[48:50], 53) // SCTP checksum bytes, pre-fix forged RemotePort + binary.BigEndian.PutUint16(pkt[50:52], 53) // pre-fix forged LocalPort + + require.NoError(t, newPacket(pkt, true, p)) + assert.Equal(t, uint8(layers.IPProtocolSCTP), p.Protocol, "must classify as the true protocol, not the forged UDP") + assert.Equal(t, uint16(0), p.RemotePort) + assert.Equal(t, uint16(0), p.LocalPort) + assert.False(t, p.Fragment) + + // Same confusion, but the unknown protocol sits after a real extension header. The HopByHop is walked + // correctly, then SCTP must still fail closed instead of being walked into its own payload. Protocol is + // the only assertion that discriminates the fix here, a regression that walked SCTP would misclassify it. + chained := make([]byte, 60) + chained[0] = 0x60 // version 6 + chained[6] = byte(layers.IPProtocolIPv6HopByHop) // NextHeader = HopByHop extension + chained[7] = 64 // hop limit + chained[40] = byte(layers.IPProtocolSCTP) // HopByHop NextHeader = SCTP + chained[41] = 0 // HopByHop length 0 -> 8 bytes, SCTP begins at offset 48 + binary.BigEndian.PutUint16(chained[48:50], 0x1100) // SCTP src port, pre-fix forged NextHeader/length bait + binary.BigEndian.PutUint16(chained[50:52], 445) // SCTP dst port, never read by parseV6 + + require.NoError(t, newPacket(chained, true, p)) + assert.Equal(t, uint8(layers.IPProtocolSCTP), p.Protocol, "must fail closed on the unknown protocol after the extension header") + assert.Equal(t, uint16(0), p.RemotePort) + assert.Equal(t, uint16(0), p.LocalPort) + assert.False(t, p.Fragment) +}