From 3cfc5c82a835c78f26f21bedde87515be332b8f4 Mon Sep 17 00:00:00 2001 From: riccardom Date: Mon, 19 Oct 2026 16:45:35 +0200 Subject: [PATCH] [client] pqkem: install strict-mode keys on idle lazy peers ML-KEM strict mode promises that no traffic crosses on a non-PQ key. Under lazy connections an idle peer is still installed in WireGuard, so that traffic towards it can wake it, and it accepts a handshake the remote initiates directly. Both lazy listeners installed it without a preshared key, so a freshly created idle peer accepted a classical session before any KEM exchange, for example from a remote whose WireGuard still targets the victim's real endpoint (security review of #7098, finding F-002). The strict sentinel only reached WireGuard later, on the normal connection path. Let the lazy peer config carry a key provider that the listeners install with the idle peer. In strict mode it yields the PQ PSK once derived and the sentinel until then; it is read at install time so it is never stale. Outside strict mode it yields nothing and the idle peer's key is left unchanged, as before. --- client/internal/conn_mgr.go | 9 +- .../lazyconn/activity/listener_bind.go | 2 +- .../lazyconn/activity/listener_psk_test.go | 106 ++++++++++++++++++ .../lazyconn/activity/listener_udp.go | 2 +- client/internal/lazyconn/peercfg.go | 14 +++ client/internal/peer/conn.go | 15 +++ client/internal/peer/conn_pq_test.go | 32 ++++++ 7 files changed, 174 insertions(+), 6 deletions(-) create mode 100644 client/internal/lazyconn/activity/listener_psk_test.go diff --git a/client/internal/conn_mgr.go b/client/internal/conn_mgr.go index 9a5d744af..a2f9a972c 100644 --- a/client/internal/conn_mgr.go +++ b/client/internal/conn_mgr.go @@ -185,10 +185,11 @@ func (e *ConnMgr) SetExcludeList(ctx context.Context, peerIDs map[string]bool) { // newLazyPeerConfig describes a peer connection to the lazy connection manager. func newLazyPeerConfig(peerKey string, conn *peer.Conn) lazyconn.PeerConfig { return lazyconn.PeerConfig{ - PublicKey: peerKey, - AllowedIPs: conn.WgConfig().AllowedIps, - PeerConnID: conn.ConnID(), - Log: conn.Log, + PublicKey: peerKey, + AllowedIPs: conn.WgConfig().AllowedIps, + PeerConnID: conn.ConnID(), + Log: conn.Log, + PresharedKey: conn.IdlePresharedKey, } } diff --git a/client/internal/lazyconn/activity/listener_bind.go b/client/internal/lazyconn/activity/listener_bind.go index 72a0cfc76..82ff16190 100644 --- a/client/internal/lazyconn/activity/listener_bind.go +++ b/client/internal/lazyconn/activity/listener_bind.go @@ -106,7 +106,7 @@ func (d *BindListener) setupLazyConn() error { IP: d.fakeIP.AsSlice(), Port: lazyBindPort, } - return d.wgIface.UpdatePeer(d.peerCfg.PublicKey, d.peerCfg.AllowedIPs, 0, endpoint, nil) + return d.wgIface.UpdatePeer(d.peerCfg.PublicKey, d.peerCfg.AllowedIPs, 0, endpoint, d.peerCfg.IdlePresharedKey()) } // ReadPackets blocks until activity is detected on the LazyConn or the listener is closed. diff --git a/client/internal/lazyconn/activity/listener_psk_test.go b/client/internal/lazyconn/activity/listener_psk_test.go new file mode 100644 index 000000000..1cf3643cb --- /dev/null +++ b/client/internal/lazyconn/activity/listener_psk_test.go @@ -0,0 +1,106 @@ +package activity + +import ( + "net" + "net/netip" + "sync" + "testing" + "time" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" + + "github.com/netbirdio/netbird/client/internal/lazyconn" +) + +// pskRecordingIface records the preshared key each idle peer is installed with. +type pskRecordingIface struct { + MocWGIface + mu sync.Mutex + psks []*wgtypes.Key +} + +func (r *pskRecordingIface) UpdatePeer(_ string, _ []netip.Prefix, _ time.Duration, _ *net.UDPAddr, psk *wgtypes.Key) error { + r.mu.Lock() + defer r.mu.Unlock() + r.psks = append(r.psks, psk) + return nil +} + +func (r *pskRecordingIface) installed() []*wgtypes.Key { + r.mu.Lock() + defer r.mu.Unlock() + return append([]*wgtypes.Key(nil), r.psks...) +} + +type idleListener interface { + ReadPackets() + Close() +} + +func closeListener(t *testing.T, l idleListener) { + t.Helper() + done := make(chan struct{}) + go func() { + l.ReadPackets() + close(done) + }() + l.Close() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("timeout waiting for ReadPackets to exit after Close") + } +} + +// TestListeners_InstallIdlePeerWithItsPresharedKey covers ML-KEM strict mode with lazy +// connections. An idle peer is installed in WireGuard so a handshake the remote initiates +// directly is accepted; installed without a key, a fresh peer accepted a classical session +// before any post-quantum exchange, breaking strict mode's promise. Both listeners must +// install the key the peer config provides, and leave it unchanged when there is none. +func TestListeners_InstallIdlePeerWithItsPresharedKey(t *testing.T) { + sentinel := wgtypes.Key{1, 2, 3} + newCfg := func(psk func() *wgtypes.Key) lazyconn.PeerConfig { + peer := &MocPeer{PeerID: "testPeer1"} + return lazyconn.PeerConfig{ + PublicKey: peer.PeerID, + PeerConnID: peer.ConnID(), + AllowedIPs: []netip.Prefix{netip.MustParsePrefix("100.64.0.2/32")}, + Log: log.WithField("peer", "testPeer1"), + PresharedKey: psk, + } + } + listeners := map[string]func(WgInterface, lazyconn.PeerConfig) (idleListener, error){ + "udp": func(iface WgInterface, cfg lazyconn.PeerConfig) (idleListener, error) { + return NewUDPListener(iface, cfg) + }, + "bind": func(iface WgInterface, cfg lazyconn.PeerConfig) (idleListener, error) { + return NewBindListener(iface, newMockEndpointManager(), cfg) + }, + } + for name, newListener := range listeners { + t.Run(name+"/with key", func(t *testing.T) { + iface := &pskRecordingIface{} + l, err := newListener(iface, newCfg(func() *wgtypes.Key { return &sentinel })) + require.NoError(t, err) + closeListener(t, l) + + installed := iface.installed() + require.Len(t, installed, 1, "the idle peer must be installed once") + require.NotNil(t, installed[0], "the idle peer must be installed with the provided key") + assert.Equal(t, sentinel, *installed[0], "the idle peer must carry the provided key") + }) + t.Run(name+"/without key", func(t *testing.T) { + iface := &pskRecordingIface{} + l, err := newListener(iface, newCfg(nil)) + require.NoError(t, err) + closeListener(t, l) + + installed := iface.installed() + require.Len(t, installed, 1, "the idle peer must be installed once") + assert.Nil(t, installed[0], "without a key the idle peer's key must be left unchanged") + }) + } +} diff --git a/client/internal/lazyconn/activity/listener_udp.go b/client/internal/lazyconn/activity/listener_udp.go index 4b7e0ddf7..ba7e15407 100644 --- a/client/internal/lazyconn/activity/listener_udp.go +++ b/client/internal/lazyconn/activity/listener_udp.go @@ -101,7 +101,7 @@ func (d *UDPListener) Close() { func (d *UDPListener) createEndpoint() error { d.peerCfg.Log.Debugf("creating lazy endpoint: %s", d.endpoint.String()) - return d.wgIface.UpdatePeer(d.peerCfg.PublicKey, d.peerCfg.AllowedIPs, 0, d.endpoint, nil) + return d.wgIface.UpdatePeer(d.peerCfg.PublicKey, d.peerCfg.AllowedIPs, 0, d.endpoint, d.peerCfg.IdlePresharedKey()) } func (d *UDPListener) newConn() (*net.UDPConn, error) { diff --git a/client/internal/lazyconn/peercfg.go b/client/internal/lazyconn/peercfg.go index 987d06a3e..7847b19e0 100644 --- a/client/internal/lazyconn/peercfg.go +++ b/client/internal/lazyconn/peercfg.go @@ -4,6 +4,7 @@ import ( "net/netip" log "github.com/sirupsen/logrus" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" "github.com/netbirdio/netbird/client/internal/peer/id" ) @@ -13,4 +14,17 @@ type PeerConfig struct { AllowedIPs []netip.Prefix PeerConnID id.ConnID Log *log.Entry + // PresharedKey returns the key to install with the idle WireGuard peer, or nil to + // leave the peer's key as it is. It is read when the peer is installed, not when the + // config is built, so it reflects keys derived after the peer joined. A nil func + // leaves the key unchanged too. + PresharedKey func() *wgtypes.Key +} + +// IdlePresharedKey is the key to install with the idle WireGuard peer (nil: unchanged). +func (c PeerConfig) IdlePresharedKey() *wgtypes.Key { + if c.PresharedKey == nil { + return nil + } + return c.PresharedKey() } diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index 51a362a58..c22b34bca 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -1141,6 +1141,21 @@ func (conn *Conn) pqPSK() (*wgtypes.Key, bool) { return &psk, true } +// IdlePresharedKey is the preshared key an idle (lazy) WireGuard peer is installed with. +// An idle peer accepts a handshake the remote initiates directly, so in ML-KEM strict +// mode it must already carry the PQ PSK, or the strict sentinel until one exists; +// installed without a key, a fresh peer would accept a classical session before any +// exchange. Outside strict mode it returns nil and leaves the idle peer's key unchanged. +func (conn *Conn) IdlePresharedKey() *wgtypes.Key { + if conn.config.PQ == nil || !conn.config.PQStrict { + return nil + } + if psk, ok := conn.pqPSK(); ok { + return psk + } + return conn.pqStrictSentinelKey +} + // presharedKey resolves the WireGuard preshared key for the peer. pqPSK is the // post-quantum PSK looked up out of band via pqPSK (nil when none is derived yet), passed // in so the manager lock is never taken under conn.mu. diff --git a/client/internal/peer/conn_pq_test.go b/client/internal/peer/conn_pq_test.go index b813a44a0..f62d2d6e6 100644 --- a/client/internal/peer/conn_pq_test.go +++ b/client/internal/peer/conn_pq_test.go @@ -6,6 +6,7 @@ import ( "time" log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" ) @@ -88,3 +89,34 @@ func TestConn_presharedKey_PQ(t *testing.T) { require.NotEqual(t, nbPSK, *got, "the sentinel must not be the ordinary key") }) } + +// TestConn_IdlePresharedKey covers the key an idle (lazy) peer is installed with. Strict +// mode must never leave an idle peer without a key, since the remote can complete a +// classical handshake with it directly; everything else leaves the idle key unchanged. +func TestConn_IdlePresharedKey(t *testing.T) { + derivedPSK, err := wgtypes.GenerateKey() + require.NoError(t, err) + sentinel, err := wgtypes.GenerateKey() + require.NoError(t, err) + + newConn := func(pq PQHandshaker, strict bool) *Conn { + c := &Conn{Log: log.WithField("peer", "pq-idle-test")} + c.config.PQ = pq + c.config.PQStrict = strict + if strict { + c.pqStrictSentinelKey = &sentinel + } + return c + } + + got := newConn(fakePQ{ok: false}, true).IdlePresharedKey() + require.NotNil(t, got, "strict mode must install a key on an idle peer") + assert.Equal(t, sentinel, *got, "before a PQ PSK exists strict mode installs the sentinel") + + got = newConn(fakePQ{psk: derivedPSK, ok: true}, true).IdlePresharedKey() + require.NotNil(t, got, "strict mode must install a key on an idle peer") + assert.Equal(t, derivedPSK, *got, "once derived, strict mode installs the PQ PSK") + + assert.Nil(t, newConn(fakePQ{ok: false}, false).IdlePresharedKey(), "non-strict mode leaves the idle key unchanged") + assert.Nil(t, newConn(nil, false).IdlePresharedKey(), "without ML-KEM the idle key is left unchanged") +}