From f239d3b6fb013f70b30f3fd84d5b9cea5ed6c8c1 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Mon, 22 Jun 2026 15:09:35 +0300 Subject: [PATCH] Fix data race between key rotation and registry signer RotateKey swapped d.identity and then zeroed the OLD ed25519 PrivateKey buffer in place, but did so outside identityMu. The registry signer closures captured the identity under identityMu.RLock(), released the lock, and only then called cur.Sign(). An in-flight signer that grabbed the old identity before the swap therefore read the very PrivateKey bytes RotateKey was concurrently zeroing -- a use-after-zero on signing material, flagged by the race detector in the lock-graph stress harness (TestConcurrentDialEncryptDecrypt) and intermittent in production. Fix: hold identityMu.RLock() across the whole Sign() in every signer closure (Start's and RotateKey's re-bind), and zero the old key under identityMu.Lock() in the same critical section as the swap. RLock and Lock are mutually exclusive, so signing and zeroing can never overlap. The key is still zeroed promptly, preserving the no-heap-lingering intent. Add TestConcurrentRotateKeyAndSign: hammers the signer closure from N goroutines while another goroutine rotates the identity, asserting every signature verifies against the pubkey snapshotted under the same lock. Reverting either half of the fix makes it fail under -race. --- pkg/daemon/daemon.go | 30 +++-- pkg/daemon/zz_rotate_key_sign_race_test.go | 127 +++++++++++++++++++++ 2 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 pkg/daemon/zz_rotate_key_sign_race_test.go diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index f9e9dd75..564e4406 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -934,9 +934,15 @@ func (d *Daemon) Start() error { // "registry: signature verification failed" on every heartbeat. if d.identity != nil { rc.SetSigner(func(challenge string) string { + // Hold identityMu.RLock across Sign(): RotateKey zeros the old + // PrivateKey buffer in place under identityMu.Lock(). Releasing + // the lock before Sign() would let an in-flight signer read the + // very bytes RotateKey is concurrently zeroing (use-after-zero + // on signing material). RLock and Lock are mutually exclusive, + // so signing and zeroing can never overlap. d.identityMu.RLock() + defer d.identityMu.RUnlock() cur := d.identity - d.identityMu.RUnlock() if cur == nil { return "" } @@ -2311,27 +2317,33 @@ func (d *Daemon) RotateKey() (map[string]interface{}, error) { return nil, fmt.Errorf("rotate_key: registry: %w", err) } + // Swap the identity AND zero the old private key under the SAME write + // Lock. Signer closures read the private key under identityMu.RLock() + // for the full duration of Sign(); RLock and Lock are mutually + // exclusive, so zeroing here can never overlap an in-flight Sign() of + // the old key. Doing the zero outside the Lock (the old behavior) raced + // with a signer that had captured the old identity before the swap. + // + // We zero the old key so it doesn't linger on the heap until GC — a + // long-lived daemon can keep it alive for hours. ed25519.PrivateKey is + // a []byte (seed || public). d.identityMu.Lock() d.identity = newID - d.identityMu.Unlock() - - // Zero the old private key so it doesn't linger on the heap - // until GC — a long-lived daemon can keep it alive for hours. - // ed25519.PrivateKey is a []byte (seed || public). for i := range current.PrivateKey { current.PrivateKey[i] = 0 } + d.identityMu.Unlock() d.tunnels.SetIdentity(newID) // The signer installed in Start() reads d.identity under d.identityMu // on every call, so this SetSigner re-bind is no longer load-bearing — // kept for symmetry with the original RotateKey contract. The closure - // here also goes through the mutex to stay consistent if d.identity - // ever rotates again. + // here holds the RLock across Sign() (same invariant as Start's signer) + // so it is safe against a future rotation's in-place key zeroing. d.regConn.SetSigner(func(c string) string { d.identityMu.RLock() + defer d.identityMu.RUnlock() cur := d.identity - d.identityMu.RUnlock() if cur == nil { return "" } diff --git a/pkg/daemon/zz_rotate_key_sign_race_test.go b/pkg/daemon/zz_rotate_key_sign_race_test.go new file mode 100644 index 00000000..9036625c --- /dev/null +++ b/pkg/daemon/zz_rotate_key_sign_race_test.go @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package daemon + +import ( + "crypto/ed25519" + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestConcurrentRotateKeyAndSign is a regression test for the rotate-key / +// signer data race: RotateKey swaps d.identity and then zeros the OLD +// PrivateKey buffer in place, while the registry signer closure reads the +// private key to produce an Ed25519 signature. If the signer releases +// identityMu before calling Sign(), an in-flight signer that captured the +// old identity reads the very bytes RotateKey is concurrently zeroing +// (use-after-zero on signing material). Under -race this manifests as a +// WRITE/READ race on the PrivateKey slice. +// +// The fix holds identityMu.RLock() across the whole Sign() in every signer +// closure, and performs the old-key zeroing under identityMu.Lock(). RLock +// and Lock are mutually exclusive, so signing and zeroing can never overlap. +// +// This test spins N goroutines hammering the production-shaped signer closure +// while another goroutine repeatedly rotates the identity. It asserts every +// signature is valid for either the old or the new public key (never a +// corrupt/partially-zeroed key), and -race must report no data races. +func TestConcurrentRotateKeyAndSign(t *testing.T) { + t.Parallel() + + reg, rc := startTestRegistry(t) + t.Cleanup(func() { reg.Close() }) + t.Cleanup(func() { rc.Close() }) + + d := New(Config{}) + d.regConn = rc + registerSelfOnRegistry(t, d) + + // The exact signer closure the daemon installs in Start() / RotateKey: + // hold identityMu.RLock() across Sign() so the read can never overlap + // RotateKey's in-place zeroing of the old key (done under Lock()). + sign := func(challenge string) (sig []byte, pub ed25519.PublicKey) { + d.identityMu.RLock() + defer d.identityMu.RUnlock() + cur := d.identity + if cur == nil { + return nil, nil + } + // Snapshot the public key under the same lock so the verify target + // matches the key that produced the signature. + pub = append(ed25519.PublicKey(nil), cur.PublicKey...) + return cur.Sign([]byte(challenge)), pub + } + + const ( + signers = 8 + rotations = 12 + challengeFm = "rotate:race:challenge" + ) + + var ( + wg sync.WaitGroup + stop atomic.Bool + corruptSigs atomic.Int64 + signCount atomic.Int64 + ) + + // Signer goroutines: hammer the signer closure and verify each signature + // against the public key snapshotted under the SAME RLock that produced + // it. A correct signer returns a (sig, pub) pair where pub verifies sig. + // A use-after-zero — the signer reading PrivateKey bytes that RotateKey + // is concurrently zeroing — corrupts the signature so ed25519.Verify + // fails (and trips the race detector). Snapshotting the pubkey under the + // same lock means the verify target always matches the signing key, so a + // verify failure can only come from torn signing material, not from a + // benign mid-rotation key change. + for i := 0; i < signers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for !stop.Load() { + sig, pub := sign(challengeFm) + if sig == nil { + continue + } + signCount.Add(1) + if !ed25519.Verify(pub, []byte(challengeFm), sig) { + corruptSigs.Add(1) + } + } + }() + } + + // Rotation goroutine: the real production write+zero path. Each rotation + // generates a fresh keypair, swaps it in under Lock, and zeros the old + // PrivateKey buffer under Lock. + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < rotations; i++ { + if _, err := d.RotateKey(); err != nil { + t.Errorf("RotateKey: %v", err) + break + } + } + stop.Store(true) + }() + + // Safety valve so a wedged rotation can't hang the suite. + done := make(chan struct{}) + go func() { wg.Wait(); close(done) }() + select { + case <-done: + case <-time.After(30 * time.Second): + stop.Store(true) + t.Fatal("concurrent rotate/sign did not finish within 30s") + } + + if c := corruptSigs.Load(); c != 0 { + t.Fatalf("got %d corrupt signatures (use-after-zero on signing key)", c) + } + if signCount.Load() == 0 { + t.Fatal("no signatures were produced; test did not exercise the signer") + } +}