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 +}