fix: acquire both read locks in a fixed order in ConstantTimeEqual

CHANGELOG.md

@@ -46,6 +46,22 @@ mark the stability commitment.
 
 ### Fixed
 
+- **`Secret.ConstantTimeEqual` and `X25519Key.ConstantTimeEqual` no longer
+  deadlock on a reversed comparison.** Both took their two read locks in
+  argument order, so `a.ConstantTimeEqual(b)` and `b.ConstantTimeEqual(a)`
+  running concurrently acquired them in opposite directions. Read locks are
+  shared, so the cycle needs a writer queued on each buffer — which a
+  writer-preferring lock makes routine, since a `Destroy` or an emergency wipe
+  is enough. Once wedged, both buffers are unreachable for `Destroy` and
+  `WipeAllSecrets` too.
+
+  Both now acquire in a fixed global order. The core orders by `janitorKey`, a
+  process-unique counter that assumes nothing about object placement;
+  `secmem-crypto` orders by buffer address, because that counter is unexported
+  and the module builds against a released core tag — using it would have meant
+  a core release plus a floor raise before the deadlock could be fixed at all.
+  Address ordering is sound while the Go GC does not relocate heap objects.
+
 - **Constructors now wipe the caller's input on failure, not only on success.**
   `NewBuffer`, `NewSyscallSafeBuffer` and `NewSecret` warn that the input is
   zeroed and must not be reused — but every error path returned with the

secmem-crypto/x25519.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"unsafe"
 
 	"golang.org/x/crypto/curve25519"
 
@@ -170,9 +171,31 @@ func (k *X25519Key) ConstantTimeEqual(other *X25519Key) bool {
 		// ErrDestroyed/ErrSealed and yield false).
 		return !k.scalarBuf.IsDestroyed() && !k.scalarBuf.IsSealed()
 	}
+	// Acquire the two read locks in a FIXED GLOBAL ORDER. Taking them in
+	// argument order is an ABBA deadlock: k.ConstantTimeEqual(other) and
+	// other.ConstantTimeEqual(k) running concurrently grab them in opposite
+	// directions, and secmem's lock is writer-preferring, so a read acquire
+	// blocks behind any queued writer — each goroutine then holds one and waits
+	// forever for the other, wedging Destroy and WipeAllSecrets with them.
+	//
+	// Ordered by buffer ADDRESS, which is not the mechanism the core uses
+	// internally: it orders by a process-unique counter, which is strictly
+	// better because it assumes nothing about object placement. That counter is
+	// not exported, and this module builds against a RELEASED core tag, so
+	// reaching for it would mean a core release plus a floor raise before this
+	// deadlock could be fixed at all. Address ordering is sound while the Go GC
+	// does not relocate heap objects, which it has never done; switch to an
+	// exported identity if one ever lands.
+	//
+	// Swapping the operands is safe because equality is symmetric.
+	first, second := k.scalarBuf, other.scalarBuf
+	//nolint:gosec // G103: ordering two locks by address; the pointers are never dereferenced through the uintptr.
+	if uintptr(unsafe.Pointer(first)) > uintptr(unsafe.Pointer(second)) {
+		first, second = second, first
+	}
 	var equal bool
-	err := k.scalarBuf.WithBytesErr(func(a []byte) error {
-		return other.scalarBuf.WithBytesErr(func(b []byte) error {
+	err := first.WithBytesErr(func(a []byte) error {
+		return second.WithBytesErr(func(b []byte) error {
 			equal = subtle.ConstantTimeCompare(a, b) == 1
 			return nil
 		})

secret.go

@@ -89,9 +89,25 @@ func (s Secret) ConstantTimeEqual(other Secret) bool {
 		// which the borrowing contract forbids (writer-preference deadlock).
 		return s.buf.WithBytes(func([]byte) {}) == nil
 	}
+	// Acquire the two read locks in a FIXED GLOBAL ORDER. Taking them in
+	// argument order is an ABBA deadlock: a.ConstantTimeEqual(b) and
+	// b.ConstantTimeEqual(a) running concurrently grab them in opposite
+	// directions, and because the lock is writer-preferring a read acquire
+	// blocks behind any queued writer — so each holds one and waits forever for
+	// the other, wedging Destroy and WipeAllSecrets on both buffers with them.
+	//
+	// Ordered by janitorKey, which is a process-unique counter (see the janitor)
+	// rather than an address: it is stable for the buffer's whole lifetime and
+	// does not assume anything about where the GC keeps objects.
+	//
+	// Swapping the operands is safe because equality is symmetric.
+	first, second := s.buf, other.buf
+	if first.janitorKey > second.janitorKey {
+		first, second = second, first
+	}
 	equal := false
-	err := s.buf.WithBytesErr(func(a []byte) error {
-		return other.buf.WithBytesErr(func(b []byte) error {
+	err := first.WithBytesErr(func(a []byte) error {
+		return second.WithBytesErr(func(b []byte) error {
 			if len(a) == len(b) {
 				equal = subtle.ConstantTimeCompare(a, b) == 1
 			}

secret_test.go

@@ -11,6 +11,7 @@ import (
 	"log/slog"
 	"strings"
 	"testing"
+	"time"
 )
 
 // Compile-time proof the leak-safe surface is actually wired to the
@@ -323,3 +324,93 @@ func TestSecret_RedactionIgnoresState(t *testing.T) {
 		t.Errorf("redacted form changed across Destroy: %q -> %q", before, after)
 	}
 }
+
+// TestSecret_ConstantTimeEqual_AcquiresInKeyOrder pins the acquisition ORDER,
+// which is the property that makes the ABBA deadlock impossible.
+//
+// Taking the two read locks in argument order deadlocks:
+// a.ConstantTimeEqual(b) and b.ConstantTimeEqual(a) running concurrently take
+// them in opposite directions. Read locks are shared, so the cycle needs a
+// writer queued on each buffer — which this package's writer-preferring lock
+// makes routine, since a Destroy or an emergency wipe is enough.
+//
+// The deadlock itself is not what is asserted here. Reproducing it needs both
+// goroutines paused BETWEEN their two acquires, and there is no hook to pause
+// them; a stress version of this test passed just as happily against the
+// unfixed code, which makes it worthless. The order is directly observable
+// instead, and it is the actual fix.
+//
+// Construction: hold an exclusive lock on the LOWER-keyed buffer, then call
+// ConstantTimeEqual with the HIGHER-keyed one as the receiver — so argument
+// order and key order disagree.
+//
+//   - ordered (fixed): the lower-keyed buffer is taken first, blocks
+//     immediately, and the higher-keyed buffer is never read-locked at all.
+//   - argument order (unfixed): the receiver is read-locked first and STAYS
+//     locked while the call blocks on the other one.
+//
+// So a reader appearing on the higher-keyed buffer is exactly the bug.
+func TestSecret_ConstantTimeEqual_AcquiresInKeyOrder(t *testing.T) {
+	if !platformHasSecureMemory {
+		t.Skip("no secure memory on this platform")
+	}
+	s1, err := NewSecret([]byte("secret-value-aaaaaaaaaaaaaaaaaaa"))
+	if err != nil {
+		t.Skipf("NewSecret: %v", err)
+	}
+	defer func() { _ = s1.buf.Destroy() }()
+	s2, err := NewSecret([]byte("secret-value-bbbbbbbbbbbbbbbbbbb"))
+	if err != nil {
+		t.Skipf("NewSecret: %v", err)
+	}
+	defer func() { _ = s2.buf.Destroy() }()
+
+	lo, hi := s1, s2
+	if lo.buf.janitorKey > hi.buf.janitorKey {
+		lo, hi = hi, lo
+	}
+
+	// Block the lower-keyed buffer so whichever side is taken first stalls
+	// there, holding the state still for inspection.
+	lo.buf.mu.lock()
+	unlocked := false
+	defer func() {
+		if !unlocked {
+			lo.buf.mu.unlock()
+		}
+	}()
+
+	started := make(chan struct{})
+	returned := make(chan struct{})
+	go func() {
+		close(started)
+		_ = hi.ConstantTimeEqual(lo) // receiver is the HIGHER key
+		close(returned)
+	}()
+	<-started
+
+	// Poll: the unfixed order takes the receiver's read lock within
+	// microseconds and holds it for the whole blocked call, so a clean window
+	// here means the lower-keyed buffer really was taken first.
+	deadline := time.Now().Add(2 * time.Second)
+	for time.Now().Before(deadline) {
+		if hi.buf.mu.readers.Load() > 0 {
+			lo.buf.mu.unlock()
+			unlocked = true
+			<-returned
+			t.Fatal("ConstantTimeEqual read-locked the higher-keyed buffer first: it acquires in " +
+				"argument order, so a concurrent reversed comparison deadlocks (ABBA)")
+		}
+		select {
+		case <-returned:
+			t.Fatal("ConstantTimeEqual returned while the lower-keyed buffer was exclusively locked; " +
+				"it cannot have taken that lock at all")
+		default:
+		}
+		time.Sleep(time.Millisecond)
+	}
+
+	lo.buf.mu.unlock()
+	unlocked = true
+	<-returned
+}