From 19ad3bb9046acf6659d1c63b651f4913695046f9 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Tue, 7 Jul 2026 14:41:01 -0500 Subject: [PATCH] correctly discard nil proto addresses (#1785) --- control_test.go | 153 +++++++++++++++++++++++++++++++++++++++++++++++ lighthouse.go | 10 +++- relay_manager.go | 18 ++++++ remote_list.go | 12 ++++ 4 files changed, 192 insertions(+), 1 deletion(-) diff --git a/control_test.go b/control_test.go index dae759a3..94ee4ee3 100644 --- a/control_test.go +++ b/control_test.go @@ -1,6 +1,8 @@ package nebula import ( + "bytes" + "log/slog" "net" "net/netip" "reflect" @@ -9,6 +11,7 @@ import ( "github.com/slackhq/nebula/cert" "github.com/slackhq/nebula/test" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestControl_GetHostInfoByVpnIp(t *testing.T) { @@ -121,3 +124,153 @@ func assertFields(t *testing.T, expected []string, actualStruct any) { assert.Equal(t, expected, fields) } + +// alwaysAllowV4/V6 are check funcs that accept every entry (including nil pointers), +// letting us inject a nil *V4AddrPort/*V6AddrPort into a RemoteList's reported cache +// the same way a malformed proto message off the wire could. +func alwaysAllowV4(netip.Addr, *V4AddrPort) bool { return true } +func alwaysAllowV6(netip.Addr, *V6AddrPort) bool { return true } + +// TestGetRelays_SkipsNilRelayAddrs proves GetRelays tolerates nil entries in the +// RelayVpnAddrs proto slice (which protoAddrToNetAddr would nil-deref on) and still +// returns the valid relays, including the legacy OldRelayVpnAddrs. +func TestGetRelays_SkipsNilRelayAddrs(t *testing.T) { + good := netip.MustParseAddr("10.0.0.9") + + d := &NebulaMetaDetails{ + OldRelayVpnAddrs: []uint32{0x0a000001}, // 10.0.0.1 + RelayVpnAddrs: []*Addr{ + nil, + netAddrToProtoAddr(good), + nil, + }, + } + + var relays []netip.Addr + require.NotPanics(t, func() { relays = d.GetRelays() }) + + assert.Equal(t, []netip.Addr{ + netip.MustParseAddr("10.0.0.1"), + good, + }, relays) +} + +// TestGetRelays_AllNil ensures an all-nil RelayVpnAddrs slice yields no relays and no panic. +func TestGetRelays_AllNil(t *testing.T) { + d := &NebulaMetaDetails{RelayVpnAddrs: []*Addr{nil, nil}} + var relays []netip.Addr + require.NotPanics(t, func() { relays = d.GetRelays() }) + assert.Empty(t, relays) +} + +// TestRemoteList_CopyCache_SkipsNilReported proves CopyCache skips nil reported +// pointers (v4 and v6) instead of nil-dereferencing them in protoV*AddrPortToNetAddrPort. +func TestRemoteList_CopyCache_SkipsNilReported(t *testing.T) { + owner := netip.MustParseAddr("10.0.0.1") + rl := NewRemoteList([]netip.Addr{owner}, nil) + + rl.unlockedSetV4(owner, owner, []*V4AddrPort{ + nil, + newIp4AndPortFromString("1.2.3.4:5"), + nil, + }, alwaysAllowV4) + + rl.unlockedSetV6(owner, owner, []*V6AddrPort{ + nil, + newIp6AndPortFromString("[1::1]:6"), + nil, + }, alwaysAllowV6) + + var cm *CacheMap + require.NotPanics(t, func() { cm = rl.CopyCache() }) + + c := (*cm)[owner.String()] + require.NotNil(t, c) + assert.ElementsMatch(t, []netip.AddrPort{ + netip.MustParseAddrPort("1.2.3.4:5"), + netip.MustParseAddrPort("[1::1]:6"), + }, c.Reported) +} + +// TestRemoteList_Rebuild_SkipsNilReported drives unlockedCollect (via Rebuild) with +// nil reported entries and confirms only the valid addresses survive, with no panic. +func TestRemoteList_Rebuild_SkipsNilReported(t *testing.T) { + owner := netip.MustParseAddr("10.0.0.1") + rl := NewRemoteList([]netip.Addr{owner}, nil) + + rl.unlockedSetV4(owner, owner, []*V4AddrPort{ + nil, + newIp4AndPortFromString("1.2.3.4:5"), + }, alwaysAllowV4) + rl.unlockedSetV6(owner, owner, []*V6AddrPort{ + newIp6AndPortFromString("[1::1]:6"), + nil, + }, alwaysAllowV6) + + require.NotPanics(t, func() { rl.Rebuild([]netip.Prefix{}) }) + + assert.ElementsMatch(t, []netip.AddrPort{ + netip.MustParseAddrPort("1.2.3.4:5"), + netip.MustParseAddrPort("[1::1]:6"), + }, rl.addrs) +} + +// newRelayControl marshals a NebulaControl the way it arrives on the wire so we can feed +// it through HandleControlMsg's unmarshal + validate path. +func newRelayControl(t *testing.T, typ NebulaControl_MessageType, from, to *Addr) []byte { + t.Helper() + msg := &NebulaControl{ + Type: typ, + RelayFromAddr: from, + RelayToAddr: to, + } + b, err := msg.Marshal() + require.NoError(t, err) + return b +} + +// TestRelayManager_HandleControlMsg_NilRelayAddrs verifies the validation block added to +// HandleControlMsg: CreateRelay{Request,Response} carrying a nil RelayFromAddr or +// RelayToAddr are dropped with a debug log rather than nil-dereferencing downstream. +func TestRelayManager_HandleControlMsg_NilRelayAddrs(t *testing.T) { + good := netAddrToProtoAddr(netip.MustParseAddr("10.0.0.9")) + + cases := []struct { + name string + typ NebulaControl_MessageType + from *Addr + to *Addr + wantLog string // debug substring expected, "" == expect no drop log + }{ + {"request nil from", NebulaControl_CreateRelayRequest, nil, good, "nil RelayFromAddr"}, + {"request nil to", NebulaControl_CreateRelayRequest, good, nil, "nil RelayToAddr"}, + {"request both nil", NebulaControl_CreateRelayRequest, nil, nil, "nil RelayFromAddr"}, + {"response nil from", NebulaControl_CreateRelayResponse, nil, good, "nil RelayFromAddr"}, + {"response nil to", NebulaControl_CreateRelayResponse, good, nil, "nil RelayToAddr"}, + // A non-relay control type is not subject to the relay-addr validation and must + // pass through it untouched (the final switch simply no-ops on it). + {"unrelated type nil addrs", NebulaControl_None, nil, nil, ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + l := test.NewLoggerWithOutputAndLevel(&buf, slog.LevelDebug) + rm := &relayManager{l: l, hostmap: newHostMap(l)} + rm.useRelays.Store(true) + + f := &Interface{l: l} + h := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("10.0.0.2")}, localIndexId: 1} + + d := newRelayControl(t, tc.typ, tc.from, tc.to) + + require.NotPanics(t, func() { rm.HandleControlMsg(h, d, f) }) + + if tc.wantLog == "" { + assert.NotContains(t, buf.String(), "nil Relay") + } else { + assert.Contains(t, buf.String(), tc.wantLog) + } + }) + } +} diff --git a/lighthouse.go b/lighthouse.go index d23e84b8..5557b1c4 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -1418,6 +1418,9 @@ func (lhh *LightHouseHandler) handleHostPunchNotification(n *NebulaMeta, fromVpn remoteAllowList := lhh.lh.GetRemoteAllowList() for _, a := range n.Details.V4AddrPorts { + if a == nil { + continue + } b := protoV4AddrPortToNetAddrPort(a) if remoteAllowList.Allow(detailsVpnAddr, b.Addr()) { lhh.lh.punchy.Schedule(b, detailsVpnAddr) @@ -1425,6 +1428,9 @@ func (lhh *LightHouseHandler) handleHostPunchNotification(n *NebulaMeta, fromVpn } for _, a := range n.Details.V6AddrPorts { + if a == nil { + continue + } b := protoV6AddrPortToNetAddrPort(a) if remoteAllowList.Allow(detailsVpnAddr, b.Addr()) { lhh.lh.punchy.Schedule(b, detailsVpnAddr) @@ -1494,7 +1500,9 @@ func (d *NebulaMetaDetails) GetRelays() []netip.Addr { if len(d.RelayVpnAddrs) > 0 { for _, r := range d.RelayVpnAddrs { - relays = append(relays, protoAddrToNetAddr(r)) + if r != nil { + relays = append(relays, protoAddrToNetAddr(r)) + } } } return relays diff --git a/relay_manager.go b/relay_manager.go index 3d396883..318a9f1a 100644 --- a/relay_manager.go +++ b/relay_manager.go @@ -309,6 +309,22 @@ func (rm *relayManager) HandleControlMsg(h *HostInfo, d []byte, f *Interface) { v = cert.Version2 } + // validate: + switch msg.Type { + case NebulaControl_CreateRelayRequest, NebulaControl_CreateRelayResponse: + if msg.RelayFromAddr == nil { + if f.l.Enabled(context.Background(), slog.LevelDebug) { + h.logger(f.l).Debug("Control message received with nil RelayFromAddr", "type", msg.Type) + } + return + } else if msg.RelayToAddr == nil { + if f.l.Enabled(context.Background(), slog.LevelDebug) { + h.logger(f.l).Debug("Control message received with nil RelayToAddr", "type", msg.Type) + } + return + } + } + switch msg.Type { case NebulaControl_CreateRelayRequest: rm.handleCreateRelayRequest(v, h, f, msg) @@ -318,6 +334,7 @@ func (rm *relayManager) HandleControlMsg(h *HostInfo, d []byte, f *Interface) { } func (rm *relayManager) handleCreateRelayResponse(v cert.Version, h *HostInfo, f *Interface, m *NebulaControl) { + //nil-checks for protoAddrToNetAddr handled by caller relayFrom := protoAddrToNetAddr(m.RelayFromAddr) relayTo := protoAddrToNetAddr(m.RelayToAddr) rm.l.Info("handleCreateRelayResponse", @@ -399,6 +416,7 @@ func (rm *relayManager) handleCreateRelayResponse(v cert.Version, h *HostInfo, f } func (rm *relayManager) handleCreateRelayRequest(v cert.Version, h *HostInfo, f *Interface, m *NebulaControl) { + //nil-checks for protoAddrToNetAddr handled by caller from := protoAddrToNetAddr(m.RelayFromAddr) target := protoAddrToNetAddr(m.RelayToAddr) diff --git a/remote_list.go b/remote_list.go index ef6eb794..9d1b387e 100644 --- a/remote_list.go +++ b/remote_list.go @@ -344,6 +344,9 @@ func (r *RemoteList) CopyCache() *CacheMap { } for _, a := range mc.v4.reported { + if a == nil { + continue + } c.Reported = append(c.Reported, protoV4AddrPortToNetAddrPort(a)) } } @@ -354,6 +357,9 @@ func (r *RemoteList) CopyCache() *CacheMap { } for _, a := range mc.v6.reported { + if a == nil { + continue + } c.Reported = append(c.Reported, protoV6AddrPortToNetAddrPort(a)) } } @@ -582,6 +588,9 @@ func (r *RemoteList) unlockedCollect() { } for _, v := range c.v4.reported { + if v == nil { + continue + } u := protoV4AddrPortToNetAddrPort(v) if !r.unlockedIsBad(u) { addrs = append(addrs, u) @@ -598,6 +607,9 @@ func (r *RemoteList) unlockedCollect() { } for _, v := range c.v6.reported { + if v == nil { + continue + } u := protoV6AddrPortToNetAddrPort(v) if !r.unlockedIsBad(u) { addrs = append(addrs, u)