From e3efe0c53d331c31449c832863ae55cd34d0918e Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Wed, 2 Sep 2026 14:45:22 -0400 Subject: [PATCH] multiport: carry a lost install race's counter to the surviving lane window Two routines can race on a lane's first packet and derive a session each. Only one gets installed, and the loser was dropped along with the replay-window entry for the packet it had just accepted, leaving that one counter replayable. The two sessions hold identical keys, so the winner's window is the same window in every respect that matters: mark the counter seen on it instead. Closes the gap without putting a lock on the decrypt path. --- connection_state.go | 9 +++++++++ lanes.go | 23 +++++++++++++---------- lanes_test.go | 11 ++++++++--- outside.go | 2 +- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/connection_state.go b/connection_state.go index afadde87..5a3e3b64 100644 --- a/connection_state.go +++ b/connection_state.go @@ -168,6 +168,15 @@ func (cs *ConnectionState) Decrypt(l *slog.Logger, messageCounter uint64, packet return out, nil } +// noteSeen records a counter that some other session for the same keys already +// accepted, so a packet doesn't become replayable just because the session that +// decrypted it was thrown away. See laneSet.installSession, its only caller. +func (cs *ConnectionState) noteSeen(l *slog.Logger, messageCounter uint64) { + cs.decryptLock.Lock() + cs.window.Update(l, messageCounter) + cs.decryptLock.Unlock() +} + func (cs *ConnectionState) VerifyRelay(l *slog.Logger, messageCounter uint64, packet []byte, nb []byte) error { cs.decryptLock.Lock() result := cs.window.Check(l, messageCounter) diff --git a/lanes.go b/lanes.go index e86288f7..bf695fc8 100644 --- a/lanes.go +++ b/lanes.go @@ -240,8 +240,8 @@ func lanePortOffset(myAddr, peerAddr netip.Addr, peerPortCount uint16) uint16 { // deliberately left out of it: anyone who can spoof this tunnel's local index // can name any lane, and installing on sight would let them make us hold a // replay window and two cipher states per lane without authenticating anything. -// The caller installs with installSession once the packet decrypts, which is the -// first moment the lane is known to be real. +// The caller must install with installSession once the packet decrypts, which is +// the first moment the lane is known to be real. func (i *HostInfo) laneSession(s uint8) (ci *ConnectionState, cached bool, err error) { ls := i.lanes if ls == nil || s == 0 || int(s) >= len(ls.sessions) { @@ -260,15 +260,18 @@ func (i *HostInfo) laneSession(s uint8) (ci *ConnectionState, cached bool, err e } // installSession publishes a session derived by laneSession, so the next packet -// on the lane doesn't have to derive it again. cs must have already decrypted a -// packet from the peer. +// on the lane doesn't have to derive it again. cs must have already decrypted the +// packet at messageCounter. // -// A loser of the race keeps the session it decrypted with and drops it after, -// which loses that one packet's replay-window entry. The alternative is holding -// a lock across a decrypt to close a window that is one packet wide and only -// open on the first packet of a lane. -func (ls *laneSet) installSession(s uint8, cs *ConnectionState) { - ls.sessions[s].CompareAndSwap(nil, cs) +// Two routines can race on a lane's first packet and derive a session each. The +// loser's is dropped, and with it the replay-window entry for the packet it just +// accepted, so hand that counter to the session that survives — the keys are +// identical, so it is the same window in every respect that matters. +func (ls *laneSet) installSession(l *slog.Logger, s uint8, cs *ConnectionState, messageCounter uint64) { + if ls.sessions[s].CompareAndSwap(nil, cs) { + return + } + ls.sessions[s].Load().noteSeen(l, messageCounter) } // session returns lane s's session for our own use, deriving and installing it diff --git a/lanes_test.go b/lanes_test.go index 2947cc9e..d52cd6f9 100644 --- a/lanes_test.go +++ b/lanes_test.go @@ -206,7 +206,7 @@ func TestLaneSessionRxDerivation(t *testing.T) { require.NoError(t, err) assert.Equal(t, []byte("real lane traffic"), pt) - respLS.installSession(2, ci) + respLS.installSession(test.NewLogger(), 2, ci, 1) assert.Same(t, ci, respLS.sessions[2].Load()) // Now it is a hit, and the replay window the decrypt above advanced is the @@ -217,11 +217,16 @@ func TestLaneSessionRxDerivation(t *testing.T) { assert.Same(t, ci, got) // A racing install loses rather than swapping the session out, which would - // throw away the replay window the live one has been accumulating. + // throw away the replay window the live one has been accumulating. The loser's + // packet still has to be marked seen on the winner, or dropping its session + // would make that one counter replayable. other, err := newLaneConnectionState(&respLS.material, 2) require.NoError(t, err) - respLS.installSession(2, other) + require.True(t, ci.window.Check(test.NewLogger(), 7), "counter 7 seen before the race") + respLS.installSession(test.NewLogger(), 2, other, 7) assert.Same(t, ci, respLS.sessions[2].Load()) + assert.False(t, ci.window.Check(test.NewLogger(), 7), + "the loser's counter was not carried to the surviving window") } func TestLaneTxGate(t *testing.T) { diff --git a/outside.go b/outside.go index 5abef4eb..199337fe 100644 --- a/outside.go +++ b/outside.go @@ -177,7 +177,7 @@ func (f *Interface) readOutsidePackets(via ViaSender, packet []byte, rxc *rxCont if !laneCached { // The packet decrypted, so the peer really is using this lane and the // session we derived for it is worth keeping. - hostinfo.lanes.installSession(lane, ci) + hostinfo.lanes.installSession(f.l, lane, ci, h.MessageCounter) } // Roam before we respond, but only on the base tunnel: a lane's source