From 3c35e6624d90dd883f9e533dface45c9794af089 Mon Sep 17 00:00:00 2001 From: Chris Fink <7587613+deadpoets@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:14:35 -0400 Subject: [PATCH] fix: acquire both read locks in a fixed order in ConstantTimeEqual MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Secret.ConstantTimeEqual and X25519Key.ConstantTimeEqual 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 this needs a writer queued on each buffer to become a cycle — which a writer-preferring lock makes routine, since a Destroy or an emergency wipe is enough to queue one. Once wedged, the two buffers are also unreachable for Destroy and WipeAllSecrets, so one reversed comparison takes the emergency wipe down with it. Ordering keys differ by module, deliberately. The core orders by janitorKey, a process-unique counter that assumes nothing about where the GC keeps objects. secmem-crypto orders by buffer address: that counter is unexported and this module builds against a RELEASED core tag, so reaching for 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, which it has never done, and the comment says to switch if an exported identity lands. Testing, and what could not be tested -------------------------------------- The deadlock itself is not what the test asserts. Reproducing it needs both goroutines paused BETWEEN their two acquires and there is no hook to pause them: a stress version passed just as happily against the unfixed code, which makes it worthless as a guard. It was written, shown to be vacuous, and deleted rather than kept for the look of coverage. The ORDER is directly observable, and it is the property that makes the cycle impossible. Holding an exclusive lock on the lower-keyed buffer and comparing with the higher-keyed one as receiver puts argument order and key order in conflict: ordered code blocks on the lower key and never touches the higher one, while argument-order code read-locks the receiver first and holds it. A reader appearing on the higher-keyed buffer is exactly the bug. Deterministic — fails in 0.9s against the unfixed code. secmem-crypto has no equivalent test. bufferRWLock is unexported, so a test in that module cannot observe which lock was taken, and nothing exported holds a secmem buffer's exclusive lock long enough to arrange the conflict. Stating that rather than shipping a stress test that would prove nothing. --- CHANGELOG.md | 16 ++++++++ secmem-crypto/x25519.go | 27 +++++++++++- secret.go | 20 ++++++++- secret_test.go | 91 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac53ad4..4de7237 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/secmem-crypto/x25519.go b/secmem-crypto/x25519.go index 69ba8b2..2134e5c 100644 --- a/secmem-crypto/x25519.go +++ b/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 }) diff --git a/secret.go b/secret.go index ebf9ac7..b8a2802 100644 --- a/secret.go +++ b/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 } diff --git a/secret_test.go b/secret_test.go index 3d92031..804f432 100644 --- a/secret_test.go +++ b/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 +}