diff --git a/client/internal/pqkem/convergence_test.go b/client/internal/pqkem/convergence_test.go index c2bef61ac..14c4d2532 100644 --- a/client/internal/pqkem/convergence_test.go +++ b/client/internal/pqkem/convergence_test.go @@ -87,9 +87,13 @@ func TestManager_InitiatorRejectsOfferFromResponderRole(t *testing.T) { psk1 := wgB.psk("aaaa") require.NotEqual(t, PSK{}, psk1, "bootstrap established a PSK") - rogue, err := dA.startExchangeTest("bbbb", false, ExchangeID{}) + // Authenticated (sealed under the shared PSK) and shaped as a rotation, so only the + // role guard can stop it. + ack, err := newExchangeID() require.NoError(t, err) - require.NoError(t, dB.OnDataPathMessage("aaaa", rogue)) + rogue, err := dA.startExchangeTest("bbbb", false, ack) + require.NoError(t, err) + require.NoError(t, dB.OnDataPathMessage("aaaa", sealForPeer(t, dA, "bbbb", rogue))) assert.Equal(t, psk1, wgB.psk("aaaa"), "the initiator must not adopt a PSK from an offer sent by the role-responder") diff --git a/client/internal/pqkem/datapath_auth.go b/client/internal/pqkem/datapath_auth.go new file mode 100644 index 000000000..fdd0fad8e --- /dev/null +++ b/client/internal/pqkem/datapath_auth.go @@ -0,0 +1,48 @@ +package pqkem + +import ( + "crypto/hkdf" + "crypto/hmac" + "crypto/sha256" + "fmt" +) + +// dataPathMACLabel separates the data-path MAC key from the PSK the consumer programs +// into its channel, so the same secret never serves two purposes directly. +const dataPathMACLabel = "netbird-pq-datapath-mac-v1" + +// dataPathTagSize is the length of the HMAC-SHA256 tag appended to every data-path frame. +const dataPathTagSize = sha256.Size + +// dataPathMACKey derives the key that authenticates data-path messages from a PSK both +// peers hold. +func dataPathMACKey(psk PSK) ([]byte, error) { + key, err := hkdf.Key(sha256.New, psk[:], nil, dataPathMACLabel, sha256.Size) + if err != nil { + return nil, fmt.Errorf("derive data-path mac key: %w", err) + } + return key, nil +} + +// sealDataPath appends the frame's HMAC-SHA256 tag under key. +func sealDataPath(key, frame []byte) []byte { + mac := hmac.New(sha256.New, key) + mac.Write(frame) + sealed := make([]byte, 0, len(frame)+dataPathTagSize) + sealed = append(sealed, frame...) + return mac.Sum(sealed) +} + +// openDataPath verifies a sealed frame under key and returns the frame without its tag. +func openDataPath(key, sealed []byte) ([]byte, bool) { + if len(sealed) < dataPathTagSize { + return nil, false + } + frame, tag := sealed[:len(sealed)-dataPathTagSize], sealed[len(sealed)-dataPathTagSize:] + mac := hmac.New(sha256.New, key) + mac.Write(frame) + if !hmac.Equal(tag, mac.Sum(nil)) { + return nil, false + } + return frame, true +} diff --git a/client/internal/pqkem/datapath_auth_test.go b/client/internal/pqkem/datapath_auth_test.go new file mode 100644 index 000000000..b1cf42aa9 --- /dev/null +++ b/client/internal/pqkem/datapath_auth_test.go @@ -0,0 +1,137 @@ +package pqkem + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// sealForPeer seals a data-path frame under the PSK m currently shares with remoteID, as +// a genuine sender would. +func sealForPeer(t *testing.T, m *Manager, remoteID RemoteID, frame []byte) []byte { + t.Helper() + psk, ok := m.PSK(remoteID) + require.True(t, ok, "sealing needs an established PSK") + key, err := dataPathMACKey(psk) + require.NoError(t, err) + return sealDataPath(key, frame) +} + +func TestDataPathSealOpen(t *testing.T) { + key, err := dataPathMACKey(PSK{1}) + require.NoError(t, err) + otherKey, err := dataPathMACKey(PSK{2}) + require.NoError(t, err) + frame := []byte("kem frame") + + sealed := sealDataPath(key, frame) + got, ok := openDataPath(key, sealed) + require.True(t, ok, "a frame sealed under the key must open") + assert.Equal(t, frame, got, "opening must return the original frame") + + _, ok = openDataPath(otherKey, sealed) + assert.False(t, ok, "a different key must not open the frame") + + tampered := append([]byte(nil), sealed...) + tampered[0] ^= 0x01 + _, ok = openDataPath(key, tampered) + assert.False(t, ok, "a modified frame must not open") + + _, ok = openDataPath(key, sealed[:dataPathTagSize-1]) + assert.False(t, ok, "a datagram shorter than a tag must not open") +} + +// TestManager_DataPathRejectsAuthenticatedBootstrap verifies a bootstrap is never taken +// from the data path, even with a valid MAC: the data path only carries rotations of an +// exchange that signalling already established. +func TestManager_DataPathRejectsAuthenticatedBootstrap(t *testing.T) { + dA, dB, wgA, _, _ := pair(t) + defer dA.Stop() + defer dB.Stop() + + bootstrap(t, dA, dB) + psk := wgA.psk("bbbb") + + require.NoError(t, dA.OnDataPathMessage("bbbb", sealForPeer(t, dA, "bbbb", forgedOffer(t, ExchangeID{})))) + + assert.Equal(t, psk, wgA.psk("bbbb"), "a bootstrap offer over the data path must be dropped") +} + +// TestManager_DataPathRetransmitAfterCommitIsAnswered covers a lost answer. The responder +// commits the rotated PSK when it answers, so the initiator's retransmission, still sealed +// under the previous PSK, must be accepted under that key and answered, or the two peers +// never converge. +func TestManager_DataPathRetransmitAfterCommitIsAnswered(t *testing.T) { + sw := newSwitch() + wgA, wgB := newFakeWG(), newFakeWG() + dA := NewManager("aaaa", wgA, nil) // responder + dB := NewManager("bbbb", wgB, nil) // initiator + dB.retryInterval = 20 * time.Millisecond + lbA := &loopback{ep: epA, sw: sw} + dA.Start(lbA) + dB.Start(&loopback{ep: epB, sw: sw}) + dA.AddPeer("bbbb", epB) + dB.AddPeer("aaaa", epA) + defer dA.Stop() + defer dB.Stop() + + bootstrap(t, dA, dB) + boot := wgB.psk("aaaa") + + lbA.drop.Store(true) // lose the responder's answers + dB.OnDataPathRekeyed("aaaa", 0) + require.Eventually(t, func() bool { return wgA.psk("bbbb") != boot }, time.Second, 5*time.Millisecond, + "the responder must commit the rotated PSK") + lbA.drop.Store(false) + + require.Eventually(t, func() bool { return wgB.psk("aaaa") == wgA.psk("bbbb") }, 2*time.Second, 5*time.Millisecond, + "the retransmitted offer must be answered so both peers converge") +} + +// forgedOffer builds a well-formed KEM offer that no peer authenticated: fresh KEM +// material, the given AckID, and no data-path MAC. +func forgedOffer(t *testing.T, ack ExchangeID) []byte { + t.Helper() + ini, err := NewInitiator() + require.NoError(t, err) + id, err := newExchangeID() + require.NoError(t, err) + raw, err := (&OfferMsg{ExchangeID: id, AckID: ack, KEMOffer: ini.Offer()}).Encode() + require.NoError(t, err) + return raw +} + +// TestManager_DataPathRejectsForgedOffer covers spoofed data-path datagrams. The data-path +// socket only knows the sender by source IP:port, and on a host-bound socket a datagram +// with a spoofed source can arrive from a physical interface rather than the tunnel. An +// unauthenticated offer must not re-key the responder: a forged bootstrap (zero AckID) +// used to be adopted, desynchronising the peers' PSKs. A guessed AckID is covered too. +func TestManager_DataPathRejectsForgedOffer(t *testing.T) { + guessed, err := newExchangeID() + require.NoError(t, err) + cases := map[string]ExchangeID{ + "forged bootstrap (zero AckID)": {}, + "forged rotation (guessed AckID)": guessed, + } + for name, ack := range cases { + t.Run(name, func(t *testing.T) { + dA, dB, wgA, _, _ := pair(t) // dB ("bbbb") initiator, dA ("aaaa") responder + defer dA.Stop() + defer dB.Stop() + + bootstrap(t, dA, dB) + psk := wgA.psk("bbbb") + require.NotEqual(t, PSK{}, psk, "bootstrap must establish a PSK") + + // Spoofed: it arrives from the initiator's registered data-path endpoint. + dA.onDataPathInbound(epB, forgedOffer(t, ack)) + + assert.Equal(t, psk, wgA.psk("bbbb"), "a forged data-path offer must not re-key the responder") + current, ok := dA.PSK("bbbb") + require.True(t, ok) + assert.Equal(t, psk, current, "the responder's stored PSK must be unchanged") + }) + } +} diff --git a/client/internal/pqkem/manager.go b/client/internal/pqkem/manager.go index a779d0443..7efbef267 100644 --- a/client/internal/pqkem/manager.go +++ b/client/internal/pqkem/manager.go @@ -114,6 +114,7 @@ type Manager struct { established map[RemoteID]bool // peer has completed at least one exchange failures map[RemoteID]int // consecutive rekey failures per peer psks map[RemoteID]PSK // latest derived PSK per peer (pulled at WG peer-config time) + prevPSKs map[RemoteID]PSK // PSK before the latest commit; still authenticates data-path messages sealed under it capable map[RemoteID]bool // peer runs the KEM (advertised a PQ port); false = known non-capable peerAddrs map[RemoteID]netip.AddrPort // remoteID -> data-path endpoint (send routing) peersByAddr map[netip.AddrPort]RemoteID // reverse: source endpoint -> remoteID (inbound) @@ -164,6 +165,7 @@ func NewManager(localID LocalID, h CallbackHandler, logger *slog.Logger, opts .. established: make(map[RemoteID]bool), failures: make(map[RemoteID]int), psks: make(map[RemoteID]PSK), + prevPSKs: make(map[RemoteID]PSK), capable: make(map[RemoteID]bool), peerAddrs: make(map[RemoteID]netip.AddrPort), peersByAddr: make(map[netip.AddrPort]RemoteID), @@ -297,6 +299,7 @@ func (m *Manager) RemovePeer(remoteID RemoteID) { delete(m.established, remoteID) delete(m.failures, remoteID) delete(m.psks, remoteID) + delete(m.prevPSKs, remoteID) delete(m.capable, remoteID) if ep, ok := m.peerAddrs[remoteID]; ok { delete(m.peersByAddr, ep) @@ -479,22 +482,37 @@ func (m *Manager) onDataPathInbound(src netip.AddrPort, msg []byte) { } // OnDataPathMessage handles a KEM message received over the data path from remoteID -// and pushes any reply back over the data path. +// and pushes any reply back over the data path. The sender is only known by its source +// address, which can be spoofed, so a message that does not carry a valid MAC under a PSK +// shared with remoteID is rejected. func (m *Manager) OnDataPathMessage(remoteID RemoteID, raw []byte) error { - typ, msg, err := Decode(raw) + frame, key, err := m.openDataPathFrom(remoteID, raw) + if err != nil { + return err + } + typ, msg, err := Decode(frame) if err != nil { return fmt.Errorf("decode data-path msg from %s: %w", remoteID, err) } switch typ { case MsgOffer: - answer, err := m.processOffer(remoteID, msg.(*OfferMsg), viaDataPathLabel) + o := msg.(*OfferMsg) + // The data path only carries rotations of an established exchange. A bootstrap + // starts from scratch, so it must come over the authenticated signalling channel. + if o.AckID == (ExchangeID{}) { + m.debug("pqkem: dropping a bootstrap offer received over the data path", "peer", remoteID, "exchange", idHex(o.ExchangeID)) + return nil + } + answer, err := m.processOffer(remoteID, o, viaDataPathLabel) if err != nil { return err } if answer == nil { return nil } - return m.pushDataPath(remoteID, answer) + // Seal the reply under the key that verified the offer. processOffer has just + // committed the new PSK, which the initiator only derives once it reads this answer. + return m.sendDataPath(remoteID, sealDataPath(key, answer)) case MsgAnswer: return m.processAnswer(remoteID, msg.(*AnswerMsg), viaDataPathLabel) case MsgError: @@ -567,15 +585,57 @@ func (m *Manager) OnDataPathDown(remoteID RemoteID) {} // ---- internals ---- -// commitPSKLocked records a freshly derived PSK as the peer's current one. Assumes m.mu -// is held. +// commitPSKLocked records a freshly derived PSK as the peer's current one, keeping the one +// it replaces: messages sealed under it may still be in flight (see openDataPathFrom). +// Assumes m.mu is held. func (m *Manager) commitPSKLocked(remoteID RemoteID, psk PSK) { + if old, ok := m.psks[remoteID]; ok { + m.prevPSKs[remoteID] = old + } m.psks[remoteID] = psk } -// pushDataPath sends a message to the peer over the data-path transport. +// pushDataPath sends a message to the peer over the data-path transport, sealed under +// the PSK the two peers currently share. func (m *Manager) pushDataPath(remoteID RemoteID, msg []byte) error { - return m.sendDataPath(remoteID, msg) + m.mu.Lock() + psk, ok := m.psks[remoteID] + m.mu.Unlock() + if !ok { + return fmt.Errorf("no PSK to authenticate a data-path message to %s", remoteID) + } + key, err := dataPathMACKey(psk) + if err != nil { + return err + } + return m.sendDataPath(remoteID, sealDataPath(key, msg)) +} + +// openDataPathFrom authenticates a data-path message from a peer under its current PSK, +// or failing that the previous one: an offer retransmitted after this side committed the +// PSK it derived was still sealed under the old key. It returns the frame and the key +// that verified it. +func (m *Manager) openDataPathFrom(remoteID RemoteID, raw []byte) ([]byte, []byte, error) { + m.mu.Lock() + candidates := make([]PSK, 0, 2) + if psk, ok := m.psks[remoteID]; ok { + candidates = append(candidates, psk) + } + if psk, ok := m.prevPSKs[remoteID]; ok { + candidates = append(candidates, psk) + } + m.mu.Unlock() + + for _, psk := range candidates { + key, err := dataPathMACKey(psk) + if err != nil { + return nil, nil, err + } + if frame, ok := openDataPath(key, raw); ok { + return frame, key, nil + } + } + return nil, nil, fmt.Errorf("unauthenticated data-path message from %s", remoteID) } // sendDataPath resolves the peer's endpoint and sends over the data-path transport, diff --git a/client/internal/pqkem/secret_test.go b/client/internal/pqkem/secret_test.go index 5b1c42c2d..610ff07ff 100644 --- a/client/internal/pqkem/secret_test.go +++ b/client/internal/pqkem/secret_test.go @@ -55,12 +55,19 @@ func TestManager_PresharedSecretBindsPSK(t *testing.T) { 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) + if !tc.match { + // The peers share no PSK, so the responder cannot authenticate the data-path + // rotation offer and keeps its key: a rotation cannot bridge the secret either. + assert.Never(t, func() bool { return wgA.psk("bbbb") != bootA }, 200*time.Millisecond, 10*time.Millisecond, + "a rotation between peers with different secrets must not re-key the responder") + return + } + // Rotation over the data path re-derives the PSK; it must honour the secret too. 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") + assert.Equal(t, wgA.psk("bbbb"), wgB.psk("aaaa"), "rotated PSKs must match when the secrets match") }) } }