fix: wipe constructor inputs on failure; close three wipe-guarantee overclaims - #64
Merged
Merged
Conversation
…verclaims Four findings that share one shape: the code claimed to wipe more than it did. Constructors wiped the input only on success (securebuf.go:134) ---------------------------------------------------------------- NewBuffer, NewSyscallSafeBuffer and NewSecret warn that the input is zeroed and must not be reused. Every error path returned with the plaintext intact. A caller who believes the warning does not wipe it themselves, so an allocation failure left the secret sitting in an ordinary heap slice — the worst of both. Now deferred, so a future error path cannot forget it. Verified against the unfixed code: a failing 512 MiB allocation left 536870912/536870912 plaintext bytes in the caller's slice. A retry after ErrNoSecureMemory now has nothing left to copy, and does not need one: that error depends only on a per-platform const and WithInsecureFallback, both knowable before the call via Probe. HKDF Extract ran outside the scrub window (kdf.go:219) ------------------------------------------------------- hkdf.New performs Extract — PRK = HMAC(salt, secret) — and the PRK is key-equivalent for every byte Expand goes on to produce. It was constructed before the ScrubErr window the doc says wraps the derivation, so the single most sensitive intermediate was the one value that window did not cover. Moved inside. MarshalOpenSSHPrivateKey claimed copies it cannot reach (ssh.go:107) --------------------------------------------------------------------- "This copy, and every derived form below, is wiped" was not true. ssh.MarshalPrivateKey builds its own intermediates around the private key and returns only the final slice, so those copies are unreachable from here. The comment now states what is covered and what is not, which is the honest version. One of them was removable: pem.EncodeToMemory grows a bytes.Buffer as it writes, and every growth orphans an array holding a prefix of the base64-encoded PRIVATE KEY. Encoding into a pre-grown buffer means one array, which the existing defer wipes in full. Scrub's abandoned stack segment (scrub_legacy.go:26) ----------------------------------------------------- "Nothing sensitive is on the abandoned copy" holds only if the caller had nothing sensitive on its stack, which is not what this package is for. morestack copies the WHOLE stack, so the segment freed by the entry wipe's own growth carries the caller's pre-existing stack and returns to the pool unwiped, unreachable from Go. Now in the documented limits rather than asserted away. Also corrects the stale "a no-op on other architectures", untrue since scrubframe_arm64.s landed.
deadpoets
added a commit
that referenced
this pull request
Aug 19, 2026
…rting The comment added in #63 contains a literal empty `${{ }}`. Actions expands expressions in `run:` before any shell sees them, so the `#` protects nothing and the file has been unparseable since f6218b4 — zero-job runs against pushes, nothing on any PR's checks, so #64/#65/#66 all looked green.
deadpoets
added a commit
that referenced
this pull request
Aug 19, 2026
## The soak has not run since #63 The comment I added in #63 contains a literal empty `${{ }}`. Actions expands expressions in `run:` before any shell sees them, so the `#` protects nothing — the file has been unparseable ever since. ``` last success 2026-08-19T00:27:43Z workflow_dispatch 704806f first failure 2026-08-19T01:54:52Z push f6218b4 (#63) ``` It fails invisibly: a zero-job run against a **push**, never against the PR. #64, #65 and #66 each reported 22/22 green while the soak was dead. ## Commits 1. **`8b18b5a`** — drop the delimiter. Verified: dispatched run [32210902399](https://github.com/deadpoets/secmem/actions/runs/32210902399) completed successfully, 20 iterations. 2. **`390695d`** — `actionlint` in the `lint` job, pinned to v1.7.12. Verified to exit 1 on the pre-fix file and 0 on the fixed one. 3. **`ca25dbc`** — fingerprint the runner on every soak run. ## Fingerprint, first result | | workstation | `windows-latest` | |---|---|---| | OS | Win 11 Insider 26220 (client) | **Server 2025** Datacenter 26100 | | CPU | Intel Core Ultra 7 265KF, 20 logical | **AMD EPYC 7763**, 4 logical | | Mandatory ASLR | **ON** | off | | HVCI | **running** | not running | | Defender realtime | **on** | off | | Scheduler quantum | 2 | 2 — *same* | Two differences I expected turned out not to exist: both hosts are build 261xx, and both run the client quantum. Worth having measured rather than assumed. Separately: **`GOMAXPROCS=4` does not emulate a 4-CPU machine.** It caps Go's Ps while the OS still spreads sysmon, GC workers and the race detector's threads across all cores — so the ~1190 local samples were never the runner's contention profile. --------- Co-authored-by: Chris Fink <7587613+deadpoets@users.noreply.github.com>
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.
Four findings that share one shape: the code claimed to wipe more than it
did. One is a real behaviour bug; three are claims corrected to match reality,
with one of them made true instead.
1. Constructors wiped the input only on success — INFO, but the real bug here
securebuf.go:134.NewBuffer,NewSyscallSafeBufferandNewSecretwarn:Every error path returned with the plaintext intact. A caller who believes
that warning does not wipe it themselves — so an allocation failure left the
secret sitting in an ordinary heap slice they had just been told was gone.
Verified against the unfixed code:
Now deferred, so a future error path cannot forget it.
Consequence, stated: a retry after
ErrNoSecureMemoryhas nothing left tocopy. It does not need one — that error depends only on a per-platform
constand
WithInsecureFallback, both knowable before the call viaProbe.2. HKDF Extract ran outside the scrub window — MEDIUM
secmem-crypto/kdf.go:219.hkdf.Newperforms Extract —PRK = HMAC(salt, secret)— and the PRK is key-equivalent for every byteExpandgoes on to produce. It was constructed before thesecmem.ScrubErrwindow that the function's own doc says wraps the derivation.
The single most sensitive intermediate was the one value the window did not
cover. Moved inside.
3.
MarshalOpenSSHPrivateKeyclaimed copies it cannot reach — MEDIUMsecmem-crypto/ssh.go:107. "This copy, and every derived form below, iswiped" was not true:
ssh.MarshalPrivateKeybuilds its own intermediatesaround the private key and hands back only the final slice, so those copies are
unreachable from this package and left to the GC. The comment now names its
limit.
One of them was removable.
pem.EncodeToMemorygrows abytes.Bufferas itwrites, and every growth orphans an array holding a prefix of the
base64-encoded private key — unreachable and unwiped. Encoding into a
pre-grown buffer means exactly one array, which the existing
deferwipes infull.
4.
Scrub's "nothing sensitive is on the abandoned copy" — LOWscrub_legacy.go:26. The entry wipe orders the stack growth, butmorestackcopies the whole stack — so the segment it abandons carries whatever the
caller already had on its stack, and returns to the stack pool unwiped and
unreachable from Go.
The claim holds only if the caller had nothing sensitive there, which is not the
situation this package exists for: a key in a local, or residue from an earlier
operation, is exactly what gets left behind. Now in the documented limits rather
than asserted away.
Also corrects the stale "a no-op on other architectures", untrue since
scrubframe_arm64.slanded.Verification
-racegreen;secmem-cryptobuild +-raceunderGOWORK=off.budget refuses, and skips if the environment happens to allow it —
inverting the repo's usual convention, since here an allocation that
unexpectedly succeeds is the environment condition.
gofmtclean.