Cap configured MTUs at 8937 so relayed sends never outgrow their buffers (#1899)

connection_state_test.go

@@ -2,6 +2,8 @@ package nebula
 
 import (
 	"net/netip"
+	"strconv"
+	"sync"
 	"testing"
 	"time"
 
@@ -11,6 +13,7 @@ import (
 	ct "github.com/slackhq/nebula/cert_test"
 	"github.com/slackhq/nebula/handshake"
 	"github.com/slackhq/nebula/header"
+	"github.com/slackhq/nebula/overlay"
 	"github.com/slackhq/nebula/test"
 	"github.com/slackhq/nebula/udp"
 	"github.com/stretchr/testify/assert"
@@ -147,6 +150,122 @@ func TestSendNoMetricsCloseTunnelKeepsRebindEpoch(t *testing.T) {
 	assert.True(t, hostinfo.markOut(1), "a CloseTunnel send must not consume the rebind epoch")
 }
 
+// capturingConn is a udp.Conn that records every WriteTo.
+type capturingConn struct {
+	udp.NoopConn
+	writes [][]byte
+	addrs  []netip.AddrPort
+}
+
+func (c *capturingConn) WriteTo(b []byte, addr netip.AddrPort) error {
+	c.writes = append(c.writes, append([]byte(nil), b...))
+	c.addrs = append(c.addrs, addr)
+	return nil
+}
+
+// TestSendNoMetricsViaRelay sends through a relay from the mtu sized buffer the cached packet flush uses. Payloads up
+// to overlay.MaxMTU go out intact, anything bigger is dropped rather than outgrowing the buffer, which once panicked.
+func TestSendNoMetricsViaRelay(t *testing.T) {
+	tests := []struct {
+		n    int
+		sent bool
+	}{
+		{0, true},
+		{1300, true},
+		{overlay.MaxMTU, true},
+		{overlay.MaxMTU + 1, false}, // too big for the relay's tag
+		{mtu - 47, false},           // too big for the inner tag, this panicked
+		{9000, false},               // a full packet at tun.mtu 9000
+	}
+	for _, tt := range tests {
+		t.Run(strconv.Itoa(tt.n), func(t *testing.T) {
+			// One tunnel to the target, carried end to end, and one to the relay that wraps it
+			targetInit, targetResp := runTestHandshake(t)
+			relayInit, relayResp := runTestHandshake(t)
+			ci, err := newConnectionStateFromResult(targetInit)
+			require.NoError(t, err)
+			relayCI, err := newConnectionStateFromResult(relayInit)
+			require.NoError(t, err)
+
+			targetAddr := netip.MustParseAddr("10.0.0.2")
+			relayAddr := netip.MustParseAddr("10.0.0.3")
+			relayRemote := netip.MustParseAddrPort("192.0.2.1:4242")
+
+			relay := &Relay{Type: TerminalType, State: Established, LocalIndex: 5, RemoteIndex: 6, PeerAddr: targetAddr}
+			relayHI := &HostInfo{
+				vpnAddrs:        []netip.Addr{relayAddr},
+				ConnectionState: relayCI,
+				relayState: RelayState{
+					relayForByAddr: map[netip.Addr]*Relay{targetAddr: relay},
+					relayForByIdx:  map[uint32]*Relay{relay.LocalIndex: relay},
+				},
+			}
+			relayHI.remote.Store(&relayRemote)
+
+			// No remote of its own, so the send has to go via the relay
+			hostinfo := &HostInfo{
+				vpnAddrs:        []netip.Addr{targetAddr},
+				ConnectionState: ci,
+				remoteIndexId:   7,
+				relayState: RelayState{
+					relays:         []netip.Addr{relayAddr},
+					relayForByAddr: map[netip.Addr]*Relay{},
+					relayForByIdx:  map[uint32]*Relay{},
+				},
+			}
+
+			l := test.NewLogger()
+			hm := newHostMap(l)
+			hm.Hosts[relayAddr] = relayHI
+			conn := &capturingConn{}
+			f := &Interface{
+				l:              l,
+				hostMap:        hm,
+				messageMetrics: &MessageMetrics{txExhausted: metrics.NewCounter()},
+				writers:        []udp.Conn{conn},
+			}
+			f.connectionManager = &connectionManager{intf: f, relayUsed: map[uint32]struct{}{}, relayUsedLock: &sync.RWMutex{}}
+
+			payload := make([]byte, tt.n)
+			for i := range payload {
+				payload[i] = byte(i)
+			}
+			counter := ci.messageCounter.Load()
+			f.sendNoMetrics(header.Message, 0, ci, hostinfo, netip.AddrPort{}, payload, make([]byte, 12), make([]byte, mtu), 0)
+
+			if !tt.sent {
+				assert.Empty(t, conn.writes)
+				assert.Equal(t, counter, ci.messageCounter.Load(), "a dropped packet must not spend a counter")
+				return
+			}
+			require.Len(t, conn.writes, 1)
+			assert.Equal(t, relayRemote, conn.addrs[0])
+			pkt := conn.writes[0]
+
+			// The relay authenticates the whole packet under its own header
+			outer := &header.H{}
+			require.NoError(t, outer.Parse(pkt))
+			assert.Equal(t, header.MessageRelay, outer.Subtype)
+			assert.Equal(t, relay.RemoteIndex, outer.RemoteIndex)
+			relayPeer, err := newConnectionStateFromResult(relayResp)
+			require.NoError(t, err)
+			require.NoError(t, relayPeer.VerifyRelay(l, outer.MessageCounter, pkt, make([]byte, 12)))
+
+			// Inside it, the target decrypts the end to end packet back to the payload
+			inner := pkt[header.Len : len(pkt)-relayPeer.dKey.Overhead()]
+			h := &header.H{}
+			require.NoError(t, h.Parse(inner))
+			assert.Equal(t, header.Message, h.Type)
+			assert.Equal(t, hostinfo.remoteIndexId, h.RemoteIndex)
+			targetPeer, err := newConnectionStateFromResult(targetResp)
+			require.NoError(t, err)
+			got, err := targetPeer.Decrypt(l, h.MessageCounter, inner, make([]byte, 12))
+			require.NoError(t, err)
+			assert.Equal(t, payload, got)
+		})
+	}
+}
+
 func TestNewConnectionStateFromResult(t *testing.T) {
 	initR, respR := runTestHandshake(t)
 

examples/config.yml

@@ -265,6 +265,7 @@ tun:
   # Sets the transmit queue length, if you notice lots of transmit drops on the tun it may help to raise this number. Default is 500
   tx_queue: 500
   # Default MTU for every packet, safe setting is (and the default) 1300 for internet based traffic
+  # Anything above 8937, here or on a route below, is capped to 8937 with a warning
   mtu: 1300
 
   # the use_offloads setting controls if Nebula will attempt to enable GSO and GRO for the tun device. Linux only, not reloadable.

header/header.go

@@ -24,6 +24,11 @@ type m = map[string]any
 const (
 	Version uint8 = 1
 	Len           = 16
+
+	// MaxOverhead is the most a payload grows on its way to the underlay.
+	// A relayed packet has two headers and two AEAD tags, one for the inner tunnel and another for the relay tunnel.
+	// Every supported cipher (today) has a 16 byte tag.
+	MaxOverhead = 2 * (Len + 16)
 )
 
 type MessageType uint8

inside.go

@@ -199,8 +199,8 @@ func (f *Interface) sendInsideMessage(hostinfo *HostInfo, pkt tio.Packet, nb []b
 		}
 
 		err = tio.SegmentSuperpacket(pkt, func(seg []byte) error {
-			//relay header + header + plaintext + AEAD tag (16 bytes for both AES-GCM and ChaCha20-Poly1305) + relay tag
-			scratch := sendBatch.Reserve(header.Len + header.Len + len(seg) + 16 + 16)
+			//relay header + header + plaintext + AEAD tag + relay tag
+			scratch := sendBatch.Reserve(len(seg) + header.MaxOverhead)
 
 			innerPacket := f.sendInsideEncrypt(hostinfo, ci, seg, scratch[header.Len:], nb)
 			if innerPacket == nil {
@@ -529,16 +529,26 @@ func (f *Interface) sendNoMetrics(t header.MessageType, st header.MessageSubType
 		return
 	}
 	useRelay := !remote.IsValid() && !hostinfo.GetRemote().IsValid()
-	fullOut := out
 
+	// The encrypt appends to out in place and a relayed packet is then wrapped in place,
+	// so out has to fit the whole packet up front or the encrypt reallocates out from under SendVia.
+	// We cap the MTU of tun devices and routes we create ourselves, so only a host that set a bigger MTU than we allow gets here.
+	need := header.Len + len(p) + ci.eKey.Overhead()
 	if useRelay {
-		if len(out) < header.Len {
-			// out always has a capacity of mtu, but not always a length greater than the header.Len.
-			// Grow it to make sure the next operation works.
-			out = out[:header.Len]
+		// The relay tunnel runs the same cipher, and adds its own header and tag
+		need += header.Len + ci.eKey.Overhead()
+	}
+	if cap(out) < need {
+		if l, ok := logging.RateLimited(f.l, f.txOversizeLimiter); ok {
+			hostinfo.logger(l).Error("Dropping outbound packet, out buffer not large enough", "outCap", cap(out), "payloadLen", len(p), "relayed", useRelay)
 		}
+		return
+	}
+
+	fullOut := out
+	if useRelay {
 		// Save a header's worth of data at the front of the 'out' buffer.
-		out = out[header.Len:]
+		out = out[header.Len:header.Len]
 	}
 
 	if noiseutil.EncryptLockNeeded {

interface.go

@@ -127,6 +127,7 @@ type Interface struct {
 
 	udpTxErrLimiter      *ratelimit.Limiter // udp direct transmit error log limiter
 	udpRelayTxErrLimiter *ratelimit.Limiter // udp relay transmit error log limiter
+	txOversizeLimiter    *ratelimit.Limiter // oversized outbound packet drop log limiter
 
 	// fatalErr holds the first unexpected reader error that caused shutdown.
 	// nil means "no fatal error" (yet)
@@ -210,6 +211,10 @@ func NewInterface(ctx context.Context, c *InterfaceConfig) (*Interface, error) {
 	if err != nil {
 		return nil, err
 	}
+	txOversizeLimiter, err := ratelimit.New(1, time.Second)
+	if err != nil {
+		return nil, err
+	}
 
 	if c.routines <= 1 {
 		c.PinThreads = false //pinning is not useful unless there's more than one tun reader
@@ -245,6 +250,7 @@ func NewInterface(ctx context.Context, c *InterfaceConfig) (*Interface, error) {
 		pinThreads:            c.PinThreads,
 		udpTxErrLimiter:       udpTxErrLimiter,
 		udpRelayTxErrLimiter:  udpRelayTxErrLimiter,
+		txOversizeLimiter:     txOversizeLimiter,
 
 		metricHandshakes: metrics.GetOrRegisterHistogram("handshakes", nil, metrics.NewExpDecaySample(1028, 0.015)),
 		metricTxDropped:  metrics.GetOrRegisterCounter("udp.tx.dropped", nil),

outside.go

@@ -160,9 +160,7 @@ func (f *Interface) readOutsidePackets(via ViaSender, packet []byte, rxc *rxCont
 		case header.TestReply:
 			// No-op, useful for the Roaming and connectionManager side-effects above
 		case header.TestRequest:
-			const maxCipherOverhead = 16 //todo we use this too often, needs a real importable const
-			const maxOverhead = header.Len + header.Len + maxCipherOverhead + maxCipherOverhead
-			if maxOverhead+len(out) > len(rxc.scratch) {
+			if header.MaxOverhead+len(out) > len(rxc.scratch) {
 				// A reply that cannot fit in scratch is dropped no matter the log level.
 				if f.l.Enabled(context.Background(), slog.LevelDebug) {
 					hostinfo.logger(f.l).Debug("dropping oversized test request", "payloadLen", len(out), "from", via)

overlay/route_test.go

@@ -6,8 +6,10 @@ import (
 	"testing"
 
 	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/header"
 	"github.com/slackhq/nebula/routing"
 	"github.com/slackhq/nebula/test"
+	"github.com/slackhq/nebula/udp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -402,3 +404,44 @@ func Test_makeMultipathUnsafeRouteTree(t *testing.T) {
 	routing.CalculateBucketsForGateways(expectedGateways)
 	assert.ElementsMatch(t, expectedGateways, r)
 }
+
+func Test_getMTU(t *testing.T) {
+	l := test.NewLogger()
+	c := config.NewC(l)
+
+	// unset
+	assert.Equal(t, DefaultMTU, getMTU(l, c))
+
+	// the largest a relayed packet can carry in one underlay buffer
+	require.Equal(t, udp.MTU, MaxMTU+header.MaxOverhead)
+	c.Settings["tun"] = map[string]any{"mtu": MaxMTU}
+	assert.Equal(t, MaxMTU, getMTU(l, c))
+
+	// too big is capped
+	c.Settings["tun"] = map[string]any{"mtu": 9000}
+	assert.Equal(t, MaxMTU, getMTU(l, c))
+}
+
+func Test_getAllRoutesFromConfigCapsMTU(t *testing.T) {
+	l := test.NewLogger()
+	c := config.NewC(l)
+	c.Settings["tun"] = map[string]any{
+		"routes": []any{
+			map[string]any{"mtu": "9000", "route": "10.0.0.0/29"},
+			map[string]any{"mtu": "1300", "route": "10.0.0.8/29"},
+		},
+		"unsafe_routes": []any{
+			map[string]any{"via": "127.0.0.1", "mtu": "9000", "route": "1.0.0.0/29"},
+		},
+	}
+
+	changed, routes, err := getAllRoutesFromConfig(l, c, []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")}, true)
+	require.NoError(t, err)
+	assert.True(t, changed)
+
+	mtus := map[string]int{}
+	for _, r := range routes {
+		mtus[r.Cidr.String()] = r.MTU
+	}
+	assert.Equal(t, map[string]int{"10.0.0.0/29": MaxMTU, "10.0.0.8/29": 1300, "1.0.0.0/29": MaxMTU}, mtus)
+}

overlay/tun.go

@@ -7,11 +7,27 @@ import (
 	"net/netip"
 
 	"github.com/slackhq/nebula/config"
+	"github.com/slackhq/nebula/header"
+	"github.com/slackhq/nebula/udp"
 	"github.com/slackhq/nebula/util"
 )
 
 const DefaultMTU = 1300
 
+// MaxMTU is the largest tun.mtu, or route mtu, the underlay can carry.
+// Every underlay buffer is udp.MTU bytes, and a relayed packet adds header.MaxOverhead to its plaintext.
+const MaxMTU = udp.MTU - header.MaxOverhead
+
+// getMTU reads tun.mtu, capping a value too big for the underlay to carry.
+func getMTU(l *slog.Logger, c *config.C) int {
+	mtu := c.GetInt("tun.mtu", DefaultMTU)
+	if mtu > MaxMTU {
+		l.Warn("tun.mtu is too big for the underlay to carry, capping it", "mtu", mtu, "maxMTU", MaxMTU)
+		return MaxMTU
+	}
+	return mtu
+}
+
 type NameError struct {
 	Name       string
 	Underlying error
@@ -41,7 +57,7 @@ func NewFdDeviceFromConfig(fd *int) DeviceFactory {
 	}
 }
 
-func getAllRoutesFromConfig(c *config.C, vpnNetworks []netip.Prefix, initial bool) (bool, []Route, error) {
+func getAllRoutesFromConfig(l *slog.Logger, c *config.C, vpnNetworks []netip.Prefix, initial bool) (bool, []Route, error) {
 	if !initial && !c.HasChanged("tun.routes") && !c.HasChanged("tun.unsafe_routes") {
 		return false, nil, nil
 	}
@@ -57,6 +73,12 @@ func getAllRoutesFromConfig(c *config.C, vpnNetworks []netip.Prefix, initial boo
 	}
 
 	routes = append(routes, unsafeRoutes...)
+	for i, r := range routes {
+		if r.MTU > MaxMTU {
+			l.Warn("route mtu is too big for the underlay to carry, capping it", "route", r.Cidr, "mtu", r.MTU, "maxMTU", MaxMTU)
+			routes[i].MTU = MaxMTU
+		}
+	}
 	return true, routes, nil
 }
 

overlay/tun_android.go

@@ -68,7 +68,7 @@ func (t *tun) Activate() error {
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

overlay/tun_darwin.go

@@ -126,7 +126,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 		f:           os.NewFile(uintptr(fd), ""),
 		Device:      name,
 		vpnNetworks: vpnNetworks,
-		DefaultMTU:  c.GetInt("tun.mtu", DefaultMTU),
+		DefaultMTU:  getMTU(l, c),
 		l:           l,
 	}
 
@@ -167,7 +167,7 @@ func newTunFromFd(c *config.C, l *slog.Logger, deviceFd int, vpnNetworks []netip
 		f:           file,
 		Device:      utunNameFromFd(deviceFd),
 		vpnNetworks: vpnNetworks,
-		DefaultMTU:  c.GetInt("tun.mtu", DefaultMTU),
+		DefaultMTU:  getMTU(l, c),
 		hostOwned:   true,
 		l:           l,
 	}
@@ -350,7 +350,7 @@ func (t *tun) activate6(network netip.Prefix) error {
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

overlay/tun_freebsd.go

@@ -370,7 +370,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 	t := &tun{
 		Device:      deviceName,
 		vpnNetworks: vpnNetworks,
-		MTU:         c.GetInt("tun.mtu", DefaultMTU),
+		MTU:         getMTU(l, c),
 		l:           l,
 		fd:          fd,
 		shutdownR:   shutdownR,
@@ -510,7 +510,7 @@ func (t *tun) setMTU() error {
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

overlay/tun_ios.go

@@ -68,7 +68,7 @@ func (t *tun) Activate() error {
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

overlay/tun_linux.go

@@ -244,7 +244,7 @@ func newTunGeneric(c *config.C, l *slog.Logger, fd int, vnetHdr bool, offloadFla
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	routeChange, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	routeChange, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}
@@ -256,7 +256,7 @@ func (t *tun) reload(c *config.C, initial bool) error {
 	// Only the mtu changed. The routes are the same, parse them again so the ones without their own mtu pick up the
 	// new default. Without this an mtu change would remove them all
 	if !routeChange {
-		_, routes, err = getAllRoutesFromConfig(c, t.vpnNetworks, true)
+		_, routes, err = getAllRoutesFromConfig(t.l, c, t.vpnNetworks, true)
 		if err != nil {
 			return err
 		}
@@ -276,7 +276,7 @@ func (t *tun) reload(c *config.C, initial bool) error {
 
 	oldDefaultMTU := t.DefaultMTU
 	oldMaxMTU := t.MaxMTU
-	newDefaultMTU := c.GetInt("tun.mtu", DefaultMTU)
+	newDefaultMTU := getMTU(t.l, c)
 	newMaxMTU := newDefaultMTU
 	for i, r := range routes {
 		if r.MTU == 0 {

overlay/tun_netbsd.go

@@ -99,7 +99,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 		fd:          fd,
 		Device:      deviceName,
 		vpnNetworks: vpnNetworks,
-		MTU:         c.GetInt("tun.mtu", DefaultMTU),
+		MTU:         getMTU(l, c),
 		l:           l,
 	}
 
@@ -340,7 +340,7 @@ func (t *tun) doIoctlByName(ctl uintptr, value uint32) error {
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

overlay/tun_openbsd.go

@@ -90,7 +90,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t
 		fd:          fd,
 		Device:      deviceName,
 		vpnNetworks: vpnNetworks,
-		MTU:         c.GetInt("tun.mtu", DefaultMTU),
+		MTU:         getMTU(l, c),
 		l:           l,
 	}
 
@@ -319,7 +319,7 @@ func (t *tun) doIoctlByName(ctl uintptr, value uint32) error {
 }
 
 func (t *tun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

overlay/tun_tester.go

@@ -31,7 +31,7 @@ type TestTun struct {
 }
 
 func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*TestTun, error) {
-	_, routes, err := getAllRoutesFromConfig(c, vpnNetworks, true)
+	_, routes, err := getAllRoutesFromConfig(l, c, vpnNetworks, true)
 	if err != nil {
 		return nil, err
 	}

overlay/tun_windows.go

@@ -75,7 +75,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*w
 	t := &winTun{
 		Device:          deviceName,
 		vpnNetworks:     vpnNetworks,
-		MTU:             c.GetInt("tun.mtu", DefaultMTU),
+		MTU:             getMTU(l, c),
 		guid:            *guid,
 		networkCategory: cat,
 		setCategory:     setCat,
@@ -118,7 +118,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*w
 }
 
 func (t *winTun) reload(c *config.C, initial bool) error {
-	change, routes, err := getAllRoutesFromConfig(c, t.vpnNetworks, initial)
+	change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial)
 	if err != nil {
 		return err
 	}

udp/udp_rio_windows.go

@@ -30,7 +30,7 @@ func procyield(cycles uint32)
 
 const (
 	packetsPerRing = 1024
-	// Caps tun.mtu at MTU-32 direct, MTU-64 relayed, unenforced anywhere else. 17.6MB page locked per socket.
+	// Fits a relayed packet at overlay.MaxMTU, which tun.mtu and route mtus are capped to. 17.6MB page locked per socket.
 	bytesPerPacket = MTU
 	receiveSpins   = 15
 )