fix(envelope): authenticate before committing replay state (remote-DoS) (#374)

DecryptFrame advanced the per-peer replay window (MaxRecvNonce) BEFORE
running AEAD.Open. A forged PILS frame that failed authentication but
carried a maximal nonce counter pinned MaxRecvNonce at ~2^63; the failing
Open rolled back only the replay bit, never the high-water-mark. Every
subsequent genuine frame from the real peer then fell >= ReplayWindowSize
behind the pinned max and was dropped as ErrOutsideWindow, tripping the
aged-session fast-drop into permanent packet loss + rekey storms.

The attack needs only a victim node ID + a peer it has a session with
(both public in the registry) and is reachable through the beacon relay
(handleRelayDeliver -> handleEncrypted), so NAT'd victims are hit with no
spoofing and no rate limit in the way.

Fix (WireGuard-style ordering): the replay counter is only ever advanced
by an authenticated frame.

  1. read-only pre-check (Crypto.WouldAcceptNonce) rejects obvious
     replays / out-of-window frames WITHOUT mutating state, so AEAD work
     is still skipped for junk;
  2. AEAD.Open runs on frames the pre-check would accept;
  3. only a successful Open commits the nonce (CheckAndRecordNonce).

A forged high-counter frame now passes the pre-check, fails AEAD, and
returns ErrAEAD without touching MaxRecvNonce. On the success path a
post-Open re-check via CheckAndRecordNonce handles the benign case where
a concurrent authenticated frame advanced the window meanwhile.

All legitimate behaviour is preserved byte-for-byte: in-window
reordering, genuine replay rejection, outside-window divergence counting,
the salvage path, and aged-session handling.

Adds WouldAcceptNonce (read-only verdict mirror of CheckAndRecordNonce)
and a regression test that fails on the old ordering (forged frame
advances MaxRecvNonce to 2^63; next genuine frame -> ErrOutsideWindow)
and passes after (forged frame -> ErrAEAD, window untouched, genuine
frame decrypts; replay + reordering guarantees still hold).

Co-authored-by: Teodor Calin <teodor@vulturelabs.io>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: 3 people

pkg/daemon/envelope/envelope.go

@@ -122,12 +122,31 @@ func EncryptWith(store *keyexchange.Store, c *keyexchange.Crypto, plaintext []by
 // Returns a DecryptResult describing the outcome. The caller (L5/L7)
 // consults Result.Err to decide rekey requests, drop policy, etc.
 //
-// On AEAD-Open failure DecryptFrame:
-//   - rolls back the speculative replay-bit (UndoReplayBit) so future
-//     legitimate frames at the same counter can still be decoded;
-//   - increments c.DecryptFailCount under c.ReplayMu (kept on the
-//     Crypto rather than under Store.mu — c.ReplayMu is leaf-level so
-//     this preserves the lock graph invariant).
+// Ordering (authenticate-before-commit — the remote-DoS fix):
+//  1. A read-only pre-check (WouldAcceptNonce) rejects obvious replays /
+//     out-of-window frames WITHOUT mutating any replay state, so the
+//     expensive AEAD work is skipped for junk.
+//  2. AEAD.Open runs on frames the pre-check would accept.
+//  3. ONLY on a successful Open is the nonce committed to the replay
+//     window (CheckAndRecordNonce advances MaxRecvNonce / sets the bit).
+//
+// This is deliberately the WireGuard-style ordering: the replay counter is
+// only ever advanced by an authenticated frame. The previous
+// check-record-then-open ordering let an attacker who knew a victim's node
+// ID + a peer it had a session with (both public) inject one PILS frame
+// with a maximal counter: CheckAndRecordNonce pinned MaxRecvNonce at that
+// value, AEAD.Open then failed, and every subsequent genuine frame from
+// the real peer fell >= ReplayWindowSize behind the pinned max — dropped as
+// ErrOutsideWindow, tripping the aged-session fast-drop into a permanent
+// packet-loss + rekey storm. Reachable through the beacon relay
+// (handleRelayDeliver → handleEncrypted), so NAT'd victims were hit with no
+// spoofing. Advancing the window only after authentication closes it.
+//
+// On AEAD-Open failure DecryptFrame increments c.DecryptFailCount under
+// c.ReplayMu (kept on the Crypto rather than under Store.mu — c.ReplayMu is
+// leaf-level so this preserves the lock graph invariant). No replay-window
+// mutation happens on the failure path (nothing was speculatively
+// recorded), so MaxRecvNonce is untouched by a forged frame.
 //
 // The grace-gated drop decision (ShouldDropOnDecryptFail) lives on
 // keyexchange.Store; this function only signals via DecryptResult.Err.
@@ -146,56 +165,30 @@ func DecryptFrame(store *keyexchange.Store, data []byte) DecryptResult {
 
 	recvCounter := binary.BigEndian.Uint64(nonce[len(nonce)-8:])
 
+	// Step 1: read-only pre-check. Reject obvious replays / out-of-window
+	// frames before spending AEAD, but WITHOUT mutating replay state. A
+	// forged high-counter frame passes this pre-check (a new maximum is not
+	// a replay) — it is only committed in step 3, after authentication.
 	c.ReplayMu.Lock()
-	ok := c.CheckAndRecordNonce(recvCounter)
+	accept := c.WouldAcceptNonce(recvCounter)
 	maxN := c.MaxRecvNonce
 	c.ReplayMu.Unlock()
 
-	if !ok {
-		err := ErrReplay
-		if recvCounter < maxN && maxN-recvCounter >= keyexchange.ReplayWindowSize {
-			err = ErrOutsideWindow
-			// Track consecutive outside-window rejections. A sustained
-			// burst means the peer's send counter has diverged from our
-			// window's high-water-mark too far for in-band recovery —
-			// only a fresh key exchange resets both sides. The caller
-			// (L7) consults ShouldDropOnOutsideWindow to enforce the
-			// threshold + grace gate. Mirrors DecryptFailCount.
-			c.ReplayMu.Lock()
-			c.OutsideWindowCount++
-			c.ReplayMu.Unlock()
-		} else {
-			// In-window replay collision. Symmetric counterpart to
-			// OutsideWindowCount on the *other* side of the window:
-			// peer's counter is INSIDE [max-window, max] but at a
-			// position we've already marked. Sustained collisions mean
-			// the peer's send counter reset (peer restarted with a
-			// persistent X25519 identity, so no PILA was negotiated)
-			// and every frame they now produce lands on a bit we
-			// already set. Recovery is structurally identical to
-			// outside-window: gate via ShouldDropOnReplay (threshold +
-			// grace), then drop the Crypto and trigger a fresh exchange.
-			c.ReplayMu.Lock()
-			c.ReplayCount++
-			c.ReplayMu.Unlock()
-		}
-		return DecryptResult{
-			PeerNodeID:   peerNodeID,
-			Counter:      recvCounter,
-			MaxRecvNonce: maxN,
-			Err:          err,
-		}
+	if !accept {
+		return rejectResult(c, peerNodeID, recvCounter, maxN)
 	}
 
-	// H3 fix: verify sender's nodeID as AAD
+	// Step 2: authenticate. H3 fix — verify sender's nodeID as AAD.
 	aad := make([]byte, 4)
 	binary.BigEndian.PutUint32(aad, peerNodeID)
 	plaintext, err := c.AEAD.Open(nil, nonce, ciphertext, aad)
 	if err != nil {
 		store.EncryptFail.Add(1)
-		// Undo the speculative nonce record on decrypt failure.
+		// AEAD failed: the frame is unauthenticated. The pre-check above
+		// recorded NOTHING in the replay window, so there is no speculative
+		// bit to roll back and — critically — MaxRecvNonce is untouched. A
+		// forged frame therefore cannot wedge the window.
 		c.ReplayMu.Lock()
-		c.UndoReplayBit(recvCounter)
 		c.DecryptFailCount++
 		c.ReplayMu.Unlock()
 		return DecryptResult{
@@ -206,6 +199,21 @@ func DecryptFrame(store *keyexchange.Store, data []byte) DecryptResult {
 		}
 	}
 
+	// Step 3: authenticated — commit the nonce to the replay window. Use
+	// CheckAndRecordNonce (a full re-check, not a blind record) because a
+	// concurrent authenticated frame may have advanced the window between
+	// the read-only pre-check and here. If this frame lost that benign
+	// reorder/duplicate race it is rejected as replay/outside-window,
+	// exactly as the old check-then-open ordering would have rejected it.
+	c.ReplayMu.Lock()
+	committed := c.CheckAndRecordNonce(recvCounter)
+	maxN = c.MaxRecvNonce
+	c.ReplayMu.Unlock()
+
+	if !committed {
+		return rejectResult(c, peerNodeID, recvCounter, maxN)
+	}
+
 	// Successful decrypt — reset all three fault counters under one lock.
 	c.ReplayMu.Lock()
 	if c.DecryptFailCount != 0 {
@@ -226,3 +234,44 @@ func DecryptFrame(store *keyexchange.Store, data []byte) DecryptResult {
 		MaxRecvNonce: maxN,
 	}
 }
+
+// rejectResult builds the DecryptResult for a nonce the replay window
+// rejects, and bumps the matching consecutive-fault counter. Shared by the
+// read-only pre-check (step 1) and the post-AEAD commit re-check (step 3).
+// maxN is the MaxRecvNonce observed under ReplayMu at the point of
+// rejection, used both for the caller's logging and to classify the
+// rejection as in-window replay vs. outside-window divergence.
+func rejectResult(c *keyexchange.Crypto, peerNodeID uint32, recvCounter, maxN uint64) DecryptResult {
+	err := ErrReplay
+	if recvCounter < maxN && maxN-recvCounter >= keyexchange.ReplayWindowSize {
+		err = ErrOutsideWindow
+		// Track consecutive outside-window rejections. A sustained burst
+		// means the peer's send counter has diverged from our window's
+		// high-water-mark too far for in-band recovery — only a fresh key
+		// exchange resets both sides. The caller (L7) consults
+		// ShouldDropOnOutsideWindow to enforce the threshold + grace gate.
+		// Mirrors DecryptFailCount.
+		c.ReplayMu.Lock()
+		c.OutsideWindowCount++
+		c.ReplayMu.Unlock()
+	} else {
+		// In-window replay collision. Symmetric counterpart to
+		// OutsideWindowCount on the *other* side of the window: peer's
+		// counter is INSIDE [max-window, max] but at a position we've
+		// already marked. Sustained collisions mean the peer's send counter
+		// reset (peer restarted with a persistent X25519 identity, so no
+		// PILA was negotiated) and every frame they now produce lands on a
+		// bit we already set. Recovery is structurally identical to
+		// outside-window: gate via ShouldDropOnReplay (threshold + grace),
+		// then drop the Crypto and trigger a fresh exchange.
+		c.ReplayMu.Lock()
+		c.ReplayCount++
+		c.ReplayMu.Unlock()
+	}
+	return DecryptResult{
+		PeerNodeID:   peerNodeID,
+		Counter:      recvCounter,
+		MaxRecvNonce: maxN,
+		Err:          err,
+	}
+}

pkg/daemon/envelope/zz_forged_counter_dos_test.go

@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package envelope_test
+
+import (
+	"bytes"
+	"encoding/binary"
+	"errors"
+	"testing"
+
+	"github.com/pilot-protocol/pilotprotocol/pkg/daemon/envelope"
+	"github.com/pilot-protocol/pilotprotocol/pkg/daemon/keyexchange"
+)
+
+// forgeFrame builds a structurally-valid PILS frame that claims to come
+// from senderID at the given nonce counter but whose ciphertext is random
+// junk — so AEAD.Open MUST fail. Returned bytes start at the senderID
+// (i.e. the `data` shape DecryptFrame consumes, PILS magic stripped).
+func forgeFrame(senderID uint32, counter uint64) []byte {
+	// [senderID(4)][nonce(12) = prefix(4)+counter(8)][ciphertext+tag(>=16)]
+	buf := make([]byte, 4+12+16)
+	binary.BigEndian.PutUint32(buf[0:4], senderID)
+	// nonce prefix (bytes 4..8) left zero; counter in the last 8 nonce bytes.
+	binary.BigEndian.PutUint64(buf[4+4:4+12], counter)
+	// ciphertext+tag stays all-zero — guaranteed to fail GCM authentication
+	// against any real key.
+	return buf
+}
+
+// TestForgedHighCounterDoesNotWedgeWindow is the regression test for the
+// HIGH-severity remote-DoS: a forged PILS frame carrying a maximal nonce
+// counter that FAILS AEAD must NOT advance MaxRecvNonce. Before the
+// authenticate-before-commit fix, DecryptFrame recorded the nonce (pinning
+// MaxRecvNonce at ~2^63) BEFORE AEAD.Open ran; the failing Open rolled back
+// only the replay bit, leaving the high-water-mark stuck. Every subsequent
+// genuine frame then fell outside the replay window and was dropped.
+//
+// This test FAILS on the pre-fix code (the genuine follow-up frame comes
+// back ErrOutsideWindow) and PASSES after (it decrypts cleanly).
+func TestForgedHighCounterDoesNotWedgeWindow(t *testing.T) {
+	t.Parallel()
+	s := newPeerSetup(t)
+
+	// A couple of genuine frames flow first, establishing a normal, low
+	// MaxRecvNonce (counters 1, 2) on the peer side.
+	for i := 0; i < 2; i++ {
+		f, err := envelope.EncryptFrame(s.localStore, s.peerID, []byte("legit-early"))
+		if err != nil {
+			t.Fatalf("encrypt legit #%d: %v", i, err)
+		}
+		if r := envelope.DecryptFrame(s.peerStore, f[4:]); r.Err != nil {
+			t.Fatalf("decrypt legit #%d: %v", i, r.Err)
+		}
+	}
+
+	maxBefore := recvMax(s.peerStore, s.localID)
+	if maxBefore == 0 {
+		t.Fatalf("precondition: expected MaxRecvNonce advanced, got 0")
+	}
+
+	// ATTACK: inject one forged frame with a maximal counter. It claims to
+	// be from localID (a public node ID) but its ciphertext is junk, so
+	// AEAD.Open fails.
+	const forgedCounter = uint64(1) << 63
+	res := envelope.DecryptFrame(s.peerStore, forgeFrame(s.localID, forgedCounter))
+	if !errors.Is(res.Err, envelope.ErrAEAD) {
+		t.Fatalf("forged frame: err = %v, want ErrAEAD", res.Err)
+	}
+
+	// CORE ASSERTION: the forged frame must NOT have advanced the window.
+	maxAfter := recvMax(s.peerStore, s.localID)
+	if maxAfter != maxBefore {
+		t.Fatalf("forged AEAD-fail advanced MaxRecvNonce: before=%d after=%d "+
+			"(window wedged — the DoS bug)", maxBefore, maxAfter)
+	}
+
+	// END-TO-END: the next genuine frame (real counter) must still decrypt.
+	// On the buggy code MaxRecvNonce is pinned at 2^63 so this frame is
+	// ReplayWindowSize behind the max and comes back ErrOutsideWindow.
+	legit, err := envelope.EncryptFrame(s.localStore, s.peerID, []byte("post-attack"))
+	if err != nil {
+		t.Fatalf("encrypt post-attack: %v", err)
+	}
+	got := envelope.DecryptFrame(s.peerStore, legit[4:])
+	if got.Err != nil {
+		t.Fatalf("genuine frame after forged injection: err = %v, want success "+
+			"(window wedged by forged high counter)", got.Err)
+	}
+	if !bytes.Equal(got.Plaintext, []byte("post-attack")) {
+		t.Fatalf("plaintext mismatch: got %q", got.Plaintext)
+	}
+}
+
+// TestForgedInjectionPreservesReplayAndReordering asserts the fix does not
+// weaken the legitimate replay-window guarantees: genuine replays are still
+// rejected, and in-window out-of-order delivery is still accepted.
+func TestForgedInjectionPreservesReplayAndReordering(t *testing.T) {
+	t.Parallel()
+	s := newPeerSetup(t)
+
+	// Encrypt three frames up front (counters 1, 2, 3) but deliver them out
+	// of order and with a forged frame interleaved.
+	f1, _ := envelope.EncryptFrame(s.localStore, s.peerID, []byte("c1"))
+	f2, _ := envelope.EncryptFrame(s.localStore, s.peerID, []byte("c2"))
+	f3, _ := envelope.EncryptFrame(s.localStore, s.peerID, []byte("c3"))
+
+	// Deliver counter=2 first (advances max to 2).
+	if r := envelope.DecryptFrame(s.peerStore, f2[4:]); r.Err != nil {
+		t.Fatalf("deliver c2: %v", r.Err)
+	}
+
+	// Forged high-counter injection between legit frames — must be ErrAEAD
+	// and must NOT wedge the window.
+	if r := envelope.DecryptFrame(s.peerStore, forgeFrame(s.localID, uint64(1)<<62)); !errors.Is(r.Err, envelope.ErrAEAD) {
+		t.Fatalf("forged interleave: err = %v, want ErrAEAD", r.Err)
+	}
+
+	// In-window REORDERING: counter=1 arrives after 2 — still accepted.
+	if r := envelope.DecryptFrame(s.peerStore, f1[4:]); r.Err != nil {
+		t.Fatalf("in-window reorder (c1 after c2): err = %v, want success", r.Err)
+	}
+
+	// In-order continuation: counter=3 accepted.
+	if r := envelope.DecryptFrame(s.peerStore, f3[4:]); r.Err != nil {
+		t.Fatalf("deliver c3: %v", r.Err)
+	}
+
+	// GENUINE REPLAY: re-deliver counter=2 — must be rejected as a replay.
+	if r := envelope.DecryptFrame(s.peerStore, f2[4:]); !errors.Is(r.Err, envelope.ErrReplay) {
+		t.Fatalf("genuine replay of c2: err = %v, want ErrReplay", r.Err)
+	}
+	// And counter=1 replay too.
+	if r := envelope.DecryptFrame(s.peerStore, f1[4:]); !errors.Is(r.Err, envelope.ErrReplay) {
+		t.Fatalf("genuine replay of c1: err = %v, want ErrReplay", r.Err)
+	}
+}
+
+// recvMax reads the peer-side MaxRecvNonce for the Crypto keyed under
+// senderID, under the same lock DecryptFrame uses.
+func recvMax(store *keyexchange.Store, senderID uint32) uint64 {
+	c := store.Get(senderID)
+	if c == nil {
+		return 0
+	}
+	c.ReplayMu.Lock()
+	defer c.ReplayMu.Unlock()
+	return c.MaxRecvNonce
+}

pkg/daemon/keyexchange/crypto.go

@@ -262,6 +262,36 @@ func (c *Crypto) CheckAndRecordNonce(counter uint64) bool {
 	return true
 }
 
+// WouldAcceptNonce reports whether counter would be accepted by
+// CheckAndRecordNonce WITHOUT mutating any replay state. It is the
+// read-only pre-check L6 (envelope) runs before the (expensive) AEAD-Open:
+// obvious replays / out-of-window frames are rejected here so the crypto
+// work is skipped, but — crucially — nothing is recorded. The replay
+// window is only advanced by CheckAndRecordNonce, and L6 calls that ONLY
+// after AEAD.Open authenticates the frame. This ordering is what stops a
+// forged high-counter frame (which fails AEAD) from pinning MaxRecvNonce
+// and wedging every subsequent genuine frame out of the window.
+//
+// Must be called with c.ReplayMu held. The logic mirrors the accept/reject
+// verdicts of CheckAndRecordNonce exactly, minus the writes.
+func (c *Crypto) WouldAcceptNonce(counter uint64) bool {
+	if c.MaxRecvNonce == 0 {
+		return true // first packet ever
+	}
+	if counter > c.MaxRecvNonce {
+		return true // new maximum
+	}
+	// counter <= MaxRecvNonce
+	if c.MaxRecvNonce-counter >= ReplayWindowSize {
+		return false // too old (outside window)
+	}
+	bit := counter % ReplayWindowSize
+	if c.ReplayBitmap[bit/64]&(1<<(bit%64)) != 0 {
+		return false // already seen (replay)
+	}
+	return true
+}
+
 // SetReplayBit sets the replay-window bit corresponding to counter.
 // Must be called with c.ReplayMu held.
 func (c *Crypto) SetReplayBit(counter uint64) {