From a73ce119127e063df647aca4603b1e5954a3021d Mon Sep 17 00:00:00 2001 From: riccardom Date: Mon, 19 Oct 2026 16:37:25 +0200 Subject: [PATCH] [client] pqkem: bind the configured pre-shared key into the PQ PSK --pre-shared-key promises that only peers holding the same key can communicate. With ML-KEM enabled the derived PQ PSK replaces it in WireGuard, but the derivation never saw it: two peers with different configured keys, or only one with a key, still agreed on the same PQ PSK and talked. A permitted peer identity, or a stolen identity key, was enough to bypass the extra secret (security review of #7098, finding F-001). Rosenpass does not have this gap because it seeds its exchange with the configured key. Mix the configured key into the HKDF keying material, so a peer without it derives a different PSK and WireGuard fails closed. It goes through the binding shared by every derivation, so bootstrap and rotation both enforce it. The library takes it as a generic option, keeping it free of WireGuard concepts. --- client/internal/engine.go | 8 +++- client/internal/pqkem/kem.go | 24 ++++++---- client/internal/pqkem/manager.go | 24 ++++++++-- client/internal/pqkem/secret_test.go | 66 ++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 12 deletions(-) create mode 100644 client/internal/pqkem/secret_test.go diff --git a/client/internal/engine.go b/client/internal/engine.go index 4e2959e13..73fca0893 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -619,7 +619,13 @@ func (e *Engine) startPQKEMManager(publicKey wgtypes.Key) error { } }, } - e.pqkemManager = pqkem.NewManager(pqkem.LocalID(publicKey.String()), cbHandler, pqkem.NewLogger()) + var opts []pqkem.Option + if e.config.PreSharedKey != nil { + // The configured key promises that only peers holding it can communicate. The PQ + // PSK replaces it in WireGuard, so it has to carry that guarantee itself. + opts = append(opts, pqkem.WithPresharedSecret(e.config.PreSharedKey[:])) + } + e.pqkemManager = pqkem.NewManager(pqkem.LocalID(publicKey.String()), cbHandler, pqkem.NewLogger(), opts...) e.pqkemManager.Start(tr) log.Infof("pqkem: enabled (udp port %d on overlay %s)", e.pqkemManager.LocalPort(), e.config.WgAddr.IP) return nil diff --git a/client/internal/pqkem/kem.go b/client/internal/pqkem/kem.go index 90c638a7a..54de4dda3 100644 --- a/client/internal/pqkem/kem.go +++ b/client/internal/pqkem/kem.go @@ -15,9 +15,9 @@ // Combiner note: this follows draft-ietf-tls-ecdhe-mlkem for X25519MLKEM768 — on // the wire ML-KEM ‖ X25519 (the draft deliberately reversed the share order for // this group), and ML-KEM_ss ‖ X25519_ss as the KDF input. The PSK is derived with -// HKDF-SHA256 over that hybrid secret, salted with a domain-separation label and -// bound (via the HKDF info) to the full transcript and the canonicalised peer -// identities. +// HKDF-SHA256 over that hybrid secret (plus an optional pre-shared secret both peers +// hold), salted with a domain-separation label and bound (via the HKDF info) to the +// full transcript and the canonicalised peer identities. package pqkem import ( @@ -47,6 +47,10 @@ type PSK [32]byte type Binding struct { LocalID []byte RemoteID []byte + // Secret is an optional pre-shared secret both peers must hold. It is mixed into the + // derivation, so peers holding different secrets (or only one holding one) derive + // different PSKs and the consumer's channel fails closed. + Secret []byte } // Initiator holds the ephemeral secrets between Offer and Finish. @@ -144,10 +148,10 @@ func Respond(offer []byte, b Binding) (answer []byte, psk PSK, err error) { } // derivePSK runs HKDF-SHA256 over the hybrid shared secret (ML-KEM_ss ‖ X25519_ss, -// per draft-ietf-tls-ecdhe-mlkem), salted with the domain-separation label, and binds -// the result — via the HKDF info — to the full transcript (offer ‖ answer) and the -// canonicalised peer identities, so the PSK cannot be transplanted to another peer -// pair or a different exchange. +// per draft-ietf-tls-ecdhe-mlkem) plus the binding's optional pre-shared secret, salted +// with the domain-separation label, and binds the result — via the HKDF info — to the +// full transcript (offer ‖ answer) and the canonicalised peer identities, so the PSK +// cannot be transplanted to another peer pair or a different exchange. func derivePSK(ssMLKEM, ssX, offer, answer []byte, b Binding) (PSK, error) { // A PSK not bound to both peer identities could be transplanted to a different peer // pair, so refuse to derive one from an empty binding. @@ -156,9 +160,13 @@ func derivePSK(ssMLKEM, ssX, offer, answer []byte, b Binding) (PSK, error) { } lo, hi := canonicalPair(b.LocalID, b.RemoteID) - ikm := make([]byte, 0, len(ssMLKEM)+len(ssX)) + // The shared secrets have fixed lengths, so appending the optional pre-shared secret + // after them keeps the input unambiguous. It sits in the keying material rather than + // the info, so a peer without it cannot derive the PSK at all. + ikm := make([]byte, 0, len(ssMLKEM)+len(ssX)+len(b.Secret)) ikm = append(ikm, ssMLKEM...) ikm = append(ikm, ssX...) + ikm = append(ikm, b.Secret...) info := make([]byte, 0, len(offer)+len(answer)+len(lo)+len(hi)) info = append(info, offer...) diff --git a/client/internal/pqkem/manager.go b/client/internal/pqkem/manager.go index 76efa9f32..958b04748 100644 --- a/client/internal/pqkem/manager.go +++ b/client/internal/pqkem/manager.go @@ -99,6 +99,7 @@ type Manager struct { localID LocalID cbHandler CallbackHandler logger *slog.Logger + secret []byte // optional pre-shared secret bound into every derivation; immutable retryInterval time.Duration maxRetries int @@ -129,15 +130,28 @@ func (m *Manager) nextGenLocked(remoteID RemoteID) uint64 { return m.genCounter[remoteID] } +// Option configures a Manager at construction. +type Option func(*Manager) + +// WithPresharedSecret binds every derived PSK, bootstrap and rotation alike, to a secret +// both peers must already share. A peer with a different secret, or none, derives a +// different PSK, so the consumer's channel fails closed instead of bypassing the secret. +// An empty secret is the same as no option. +func WithPresharedSecret(secret []byte) Option { + return func(m *Manager) { + m.secret = append([]byte(nil), secret...) + } +} + // NewManager builds a manager for the local peer identified by its peer identity key // (used for the deterministic initiator role and the identity binding). A nil logger // falls back to slog.Default(). Install the data-path transport with Start. -func NewManager(localID LocalID, h CallbackHandler, logger *slog.Logger) *Manager { +func NewManager(localID LocalID, h CallbackHandler, logger *slog.Logger, opts ...Option) *Manager { if logger == nil { logger = slog.Default() } ctx, cancel := context.WithCancel(context.Background()) - return &Manager{ + m := &Manager{ localID: localID, cbHandler: h, logger: logger, @@ -155,6 +169,10 @@ func NewManager(localID LocalID, h CallbackHandler, logger *slog.Logger) *Manage peersByAddr: make(map[netip.AddrPort]RemoteID), genCounter: make(map[RemoteID]uint64), } + for _, opt := range opts { + opt(m) + } + return m } // Start installs the data-path transport and begins its inbound delivery. The Manager @@ -566,7 +584,7 @@ func (m *Manager) pushDataPath(remoteID RemoteID, msg []byte) error { } func (m *Manager) binding(remoteID RemoteID) Binding { - return Binding{LocalID: []byte(m.localID), RemoteID: []byte(remoteID)} + return Binding{LocalID: []byte(m.localID), RemoteID: []byte(remoteID), Secret: m.secret} } func newExchangeID() (ExchangeID, error) { diff --git a/client/internal/pqkem/secret_test.go b/client/internal/pqkem/secret_test.go new file mode 100644 index 000000000..5b1c42c2d --- /dev/null +++ b/client/internal/pqkem/secret_test.go @@ -0,0 +1,66 @@ +package pqkem + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pairWithSecrets builds a wired initiator ("bbbb") / responder ("aaaa") pair whose +// managers hold the given pre-shared secrets (nil = no secret). +func pairWithSecrets(t *testing.T, secretA, secretB []byte) (dA, dB *Manager, wgA, wgB *fakeWG) { + t.Helper() + sw := newSwitch() + wgA, wgB = newFakeWG(), newFakeWG() + dA = NewManager("aaaa", wgA, nil, WithPresharedSecret(secretA)) + dB = NewManager("bbbb", wgB, nil, WithPresharedSecret(secretB)) + dA.Start(&loopback{ep: epA, sw: sw}) + dB.Start(&loopback{ep: epB, sw: sw}) + dA.AddPeer("bbbb", epB) + dB.AddPeer("aaaa", epA) + t.Cleanup(func() { + dA.Stop() + dB.Stop() + }) + return dA, dB, wgA, wgB +} + +// TestManager_PresharedSecretBindsPSK covers the configured-secret contract (a host +// option like WireGuard's --pre-shared-key: only peers holding the same secret may +// communicate). Without binding the secret into the derivation, two peers with +// different secrets derived the same PSK and the secret was silently bypassed. The PSK +// must match only when the secrets match, for the bootstrap and for a rotation alike. +func TestManager_PresharedSecretBindsPSK(t *testing.T) { + secret1 := []byte("configured-secret-one-0123456789") + secret2 := []byte("configured-secret-two-0123456789") + cases := map[string]struct { + a, b []byte + match bool + }{ + "same secret": {a: secret1, b: secret1, match: true}, + "different secrets": {a: secret1, b: secret2, match: false}, + "only the responder has one": {a: secret1, b: nil, match: false}, + "only the initiator has one": {a: nil, b: secret1, match: false}, + "no secret": {a: nil, b: nil, match: true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + dA, dB, wgA, wgB := pairWithSecrets(t, tc.a, tc.b) + + bootstrap(t, dA, dB) + bootA, bootB := wgA.psk("bbbb"), wgB.psk("aaaa") + require.NotEqual(t, PSK{}, bootA, "responder must derive a bootstrap PSK") + require.NotEqual(t, PSK{}, bootB, "initiator must derive a bootstrap PSK") + assert.Equal(t, tc.match, bootA == bootB, "bootstrap PSKs must match only when the secrets match") + + // Rotation over the data path re-derives the PSK; it must honour the secret too. + dB.OnDataPathRekeyed("aaaa", 0) + require.Eventually(t, func() bool { + return wgA.psk("bbbb") != bootA && wgB.psk("aaaa") != bootB + }, time.Second, 5*time.Millisecond, "the rotation must re-key both peers") + assert.Equal(t, tc.match, wgA.psk("bbbb") == wgB.psk("aaaa"), "rotated PSKs must match only when the secrets match") + }) + } +}