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