fix(registry): an unrelated borrow can no longer strand another secret in the emergency wipe - #53
Closed
deadpoets wants to merge 3 commits into
Closed
fix(registry): an unrelated borrow can no longer strand another secret in the emergency wipe#53deadpoets wants to merge 3 commits into
deadpoets wants to merge 3 commits into
Conversation
…gency wipe WipeAllSecrets runs two passes: the first wipes every region whose lock is free at that instant, the second blocks on what is left. The second pass was a sequential loop over the deferred slice, and that slice is built from Go map iteration, so its order is random. tryWipeInPlace reports "not done" for a lock held at the moment it looks, including a momentary one. A buffer that was merely mid-WithBytes during the first pass therefore landed in the deferred set next to a genuinely stuck one, and when it sorted behind that one it kept its plaintext for as long as the unrelated borrow ran. That is the exact hostage situation the two-pass split exists to prevent, reintroduced by the second pass itself — and the guarantee the surrounding comment stated was consequently false. Each deferred region now gets its own goroutine, so a borrow that never returns blocks only its own buffer and the ordering cannot matter. The cost is one goroutine per region still locked when the first pass ends; that set is normally empty and is bounded by the number of live registrations. A worker pool would reintroduce the bug as soon as the stuck borrows outnumbered the workers, so the count is deliberately unbounded. The regression test forces the interleaving rather than racing for it: every idle buffer is held borrowed from before the call until well after the first pass has run, which guarantees the demotion, and the stuck borrows are never released while the assertion runs, so an idle buffer can only be zeroed if the wipe waited on it independently. Map iteration order cannot be forced, so six buffers on each side reduce the unfixed code's chance of passing to one ordering in C(12,6) = 924; that trade is documented in the test. Verified: 3/3 fail against the unfixed registry.go, 5/5 pass with the fix. Found by stressing the race-built suite at GOMAXPROCS=4 to match CI's runners, where the pre-existing sibling test failed 1 process in 40.
…l 0x00 TestSealCipher_RealOverflowStillDetected wrote a fixed 0x00 over the first canary byte to simulate an overflow. The canary pattern is 16 bytes of crypto/rand, generated once per process, so on roughly 1 run in 256 that byte is already 0x00 — the write plants nothing, Destroy correctly reports no violation, and the test fails claiming the seal/unseal round trip masked a real overflow. Observed at 1 process in 250 while stressing the race-built suite at GOMAXPROCS=4, which matches 1/256 closely enough to be the whole story. corruptCanary exists for exactly this and is already used by the other overflow proofs; its own comment names the 1/256 no-op as the hazard to avoid. This call site simply predated or missed it. No product behaviour is involved: the canary check was right every time, including on the failing runs.
The first version of this test defeated map-iteration order statistically: six buffers on each side, so the unfixed code passed only on 1 ordering in C(12,6). That works, but it is a probabilistic guard for a security property, and it needed a 250 ms sleep to place the first pass. The repo already had the primitive to do this properly. waitForWritersWaiting observes writersWaiting on a specific bufferRWLock, which distinguishes the two implementations exactly: the fixed wipe waits on every deferred region independently, so each has a writer queued on its own lock, while a sequential second pass can only ever have one queued at a time. Asserting a queued writer on BOTH locks therefore fails against the unfixed code no matter which key map iteration put first. Now 2 buffers instead of 12, no sleep, and the failure is deterministic: 3/3 fail against the pre-fix registry.go with "timed out waiting for 1 queued writers (got 0)" — landing on stuck.mu or idle.mu depending on map order, but always on one of them — and 10/10 pass with the fix, in 2.1s total.
This was referenced Aug 17, 2026
Owner
Author
|
Landed on
Plus 9e2bfbc, which adds the Rebased locally rather than squash-merged so each commit carries the maintainer's SSH signature ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects found by stressing the race-built core suite at
GOMAXPROCS=4tomatch CI's 4-CPU Windows runners. One is a real bug in a security property; the
other is a flaky test that was misreporting a working control.
1.
WipeAllSecretscould strand a secret behind an unrelated borrowwipeAllInPlaceruns two passes: the first wipes every region whose lock isfree at that instant, the second blocks on what is left. The second pass was a
sequential loop over the deferred slice, and that slice is built from Go map
iteration — so its order is random.
tryWipeInPlacereportsdone=falsefor a lock held at the moment it looks,including a momentary one. So a buffer that was merely mid-
WithBytesduring the first pass landed in the deferred set beside a genuinely stuck one,
and whenever it sorted behind that one it kept its plaintext for as long as the
unrelated borrow ran.
That is exactly the hostage situation the two-pass split exists to prevent,
reintroduced by the second pass itself — which also made the guarantee stated in
the surrounding comment false:
Fix: each deferred region is waited on in its own goroutine, so a borrow
that never returns blocks only its own buffer and the ordering cannot matter.
The goroutine count is deliberately unbounded. It is one per region still locked
when the first pass ends — normally zero, and bounded by the number of live
registrations. A worker pool would reintroduce the bug the moment stuck borrows
outnumbered workers.
Reachability: any
WipeAllSecretscall, including fromInstallTerminationWipe, whenever a second buffer was in use as the wipe began.Present since the two-pass wipe landed, so it is latent in v0.3.0/v0.3.1.
The regression test is deterministic
waitForWritersWaiting(already in the suite) observeswritersWaitingon aspecific
bufferRWLock, and that distinguishes the two implementations exactly:writer queued on its own lock;
Both buffers are borrowed before
WipeAllSecretsis called, so both areguaranteed into the deferred set — no sleep, no timing assumption. Asserting a
queued writer on both locks then fails against the unfixed code regardless
of which key map iteration put first.
Verified: 3/3 fail against the pre-fix
registry.gowithtimed out waiting for 1 queued writers (got 0)— landing onstuck.muoridle.mudepending on map order, but always on one of them — and 10/10 passwith the fix, 2.1s total.
2.
TestSealCipher_RealOverflowStillDetectedwas flaky, ~1/256Unrelated to the above, and not a product defect.
The test planted its overflow by writing a literal
0x00over the first canarybyte. The canary pattern is 16 bytes of
crypto/randgenerated once perprocess, so when that byte is already
0x00the write plants nothing,Destroycorrectly reports no violation, and the test fails claiming the cipher masked a
real overflow. Observed at 1 process in 250, which matches 1/256.
corruptCanary(probeWrite(addr, probeRead(addr)^0xFF)) exists for preciselythis and is used by the other overflow proofs; its own comment names the 1/256
no-op as the hazard. This call site just missed it.
The canary check was correct on every run, including the failing ones.
Verification
-race: green.GOMAXPROCS=4: 0 failures across 150 targetedprocesses and 60 full-suite processes, all at
-test.count=5. The siblingtest this started from previously failed 1 process in 40.
Still open, not addressed here
A
fatal error: stack not a power of 2onwindows-latestunder-race, seenonce in CI (run 31982627382) and not reproduced locally. Tracked separately and
deliberately not guessed at in this PR.