From af12740b5ac247ab1246465ba0cc5cea6d77fe17 Mon Sep 17 00:00:00 2001 From: Chris Fink <7587613+deadpoets@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:03:46 -0400 Subject: [PATCH] fix: wipe constructor inputs on failure; close three wipe-guarantee overclaims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 33 +++++++++++++++++++++++++++++++++ scrub_legacy.go | 19 ++++++++++++++++--- secmem-crypto/kdf.go | 11 +++++++++-- secmem-crypto/ssh.go | 33 ++++++++++++++++++++++++++++++--- secret.go | 2 ++ securebuf.go | 17 +++++++++++++++-- securebuf_test.go | 34 ++++++++++++++++++++++++++++++++++ 7 files changed, 139 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a72aa86..ac53ad4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,39 @@ mark the stability commitment. ### Fixed +- **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 + plaintext intact. A caller following the warning does not wipe it themselves, + so an allocation failure left the secret in an ordinary heap slice they + believed was gone. The wipe is now deferred so future error paths cannot forget + it. A retry after `ErrNoSecureMemory` therefore has nothing left to copy, and + does not need one: that error depends only on the platform and on + `WithInsecureFallback`, both knowable up front via `Probe`. + +- **`HKDFInto` now performs the Extract step inside its scrub window.** + `hkdf.New` computes the PRK — `HMAC(salt, secret)`, key-equivalent for every + byte Expand goes on to produce — and it was called *outside* the + `secmem.ScrubErr` window the doc says wraps the derivation. The single most + sensitive intermediate was the one value the window did not cover. + +- **`MarshalOpenSSHPrivateKey` no longer claims to wipe copies it cannot + reach.** The comment said "this copy, and every derived form below, is wiped"; + in fact only the forms this package holds a reference to are — + `ssh.MarshalPrivateKey` builds its own intermediates around the private key + and returns only the final slice. The claim now names its limit. The PEM step + also encodes into a pre-grown buffer instead of `pem.EncodeToMemory`, whose + growing `bytes.Buffer` orphaned an unwiped array holding a prefix of the + base64-encoded private key on every reallocation. + +- **`Scrub`'s "nothing sensitive is on the abandoned copy" was false.** The entry + wipe orders the stack growth, but `morestack` copies the *whole* stack, so the + abandoned segment carries whatever the CALLER already had on its stack — a key + in a local, or residue from an earlier operation — and returns to the stack + pool unwiped, unreachable from Go. Added to the documented limits rather than + left as a claim. Also corrects the stale "a no-op on other architectures", + which has not been true since `scrubframe_arm64.s` landed. + - **`InstallTerminationWipe` now terminates the process on Windows instead of wiping and running on.** `os.Process.Signal` there implements only `os.Kill` and rejects `os.Interrupt` and SIGTERM, and the console event that triggered diff --git a/scrub_legacy.go b/scrub_legacy.go index 2b8b1cd..d3c5a6b 100644 --- a/scrub_legacy.go +++ b/scrub_legacy.go @@ -13,7 +13,9 @@ package secmem // is backed by runtime/secret and erases the registers, stack, and heap of fn's // entire call tree with runtime cooperation. This file is the fallback used on // every other build; it cannot match that, and scrubs only fn's stack frame via -// assembly (REP STOSB + SFENCE on amd64; a no-op on other architectures). +// assembly — REP STOSB + SFENCE on amd64, and a real store loop on arm64 since +// scrubframe_arm64.s landed, so "a no-op on other architectures" now means +// every architecture except those two. // // # Why the wipe runs twice // @@ -23,8 +25,17 @@ package secmem // deferred wipe would then run on the RELOCATED stack, zeroing the fresh copy // while fn's real residue sits on the old segment the runtime just freed — // untouched. Calling the wipe once on entry forces any growth to happen BEFORE -// fn writes a secret (nothing sensitive is on the abandoned copy) and pre-cleans -// the band; the deferred call is then guaranteed to run in place. +// fn writes a secret and pre-cleans the band; the deferred call is then +// guaranteed to run in place. +// +// The entry wipe orders that growth, it does not make it free. morestack copies +// the WHOLE stack, so the abandoned segment holds a copy of the caller's stack +// as it was on entry — and an earlier version of this comment claimed nothing +// sensitive was on it. That 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 copied to the new segment and left +// behind on the old one, which returns to the stack pool unwiped. Scrub cannot +// reach it, because the runtime owns the abandoned segment and does not name it. // // # Best-effort limits (stated honestly) // @@ -34,6 +45,8 @@ package secmem // - a stack relocation triggered inside fn if fn exceeds the reserved band; // - a GC stack-shrink that frees fn's segment before the wipe (asynchronous, // runtime-owned, and unreachable from Go); +// - the stack segment abandoned by the entry wipe's own growth, which carries +// a copy of whatever the CALLER already had on its stack (see above); // - CPU or vector registers. // // None of these are fixable in pure Go without runtime support — the diff --git a/secmem-crypto/kdf.go b/secmem-crypto/kdf.go index a21ea54..872a8b7 100644 --- a/secmem-crypto/kdf.go +++ b/secmem-crypto/kdf.go @@ -193,7 +193,8 @@ func HMACSHA256Into(secret, info []byte, out *secmem.SecureBuffer) error { // directly into the locked SecureBuffer mapping, where stdlib's Key() // returns a heap-allocated slice. The reader's internal extract/expand // state (the pseudorandom key and the last HMAC block) lives in unexported -// heap fields it provides no way to wipe; the derivation is therefore +// heap fields it provides no way to wipe; the derivation — including the +// Extract step that computes the PRK, which hkdf.New performs — is therefore // wrapped in [secmem.ScrubErr], which on GOEXPERIMENT=runtimesecret builds // erases those allocations once unreachable. On other builds that state is // reclaimed by the GC but not explicitly zeroed — a residue window this @@ -216,8 +217,14 @@ func HKDFInto(h func() hash.Hash, secret, salt, info []byte, out *secmem.SecureB return fmt.Errorf("secmemcrypto: hkdf derive: output %d exceeds the RFC 5869 limit of %d bytes (255 x hash size)", size, maxOut) } - r := hkdf.New(h, secret, salt, info) err := secmem.ScrubErr(func() error { + // hkdf.New INSIDE the window, not before it. New performs the Extract + // step — PRK = HMAC(salt, secret) — and the PRK is key-equivalent for + // every byte Expand goes on to produce. Constructing the reader outside + // the scrub left that computation's stack residue and the PRK allocation + // outside the very window this function's doc says covers the + // derivation, which is the one value it most needed to cover. + r := hkdf.New(h, secret, salt, info) return out.WithBytesErr(func(dst []byte) error { _, err := io.ReadFull(r, dst) return err diff --git a/secmem-crypto/ssh.go b/secmem-crypto/ssh.go index e65ae6d..eaf57cd 100644 --- a/secmem-crypto/ssh.go +++ b/secmem-crypto/ssh.go @@ -5,6 +5,7 @@ package secmemcrypto import ( + "bytes" "crypto" "crypto/ed25519" "encoding/pem" @@ -103,8 +104,18 @@ func (s *Ed25519Signer) MarshalOpenSSHPrivateKey(comment string) (*secmem.Secure err := secmem.ScrubErr(func() error { return s.seedBuf.WithBytesErr(func(seed []byte) error { // ed25519.NewKeyFromSeed's FIPS self-check panics on a mmap'd - // (off-heap) input — copy to an ordinary heap slice first. This - // copy, and every derived form below, is wiped before returning. + // (off-heap) input — copy to an ordinary heap slice first. + // + // Every derived form this function can REACH is wiped before + // returning: seedCopy, priv, block.Bytes and the encoded PEM. That + // is not the same as "every copy of the key is wiped", which an + // earlier version of this comment claimed. ssh.MarshalPrivateKey + // builds its own intermediates around the private key — the marshal + // scratch and the padded key block — and hands back only the final + // slice, so those copies are unreachable from here and are left to + // the GC. Naming the limit is the honest version; the ScrubErr + // window above is what narrows it, and on + // GOEXPERIMENT=runtimesecret builds erases them once unreachable. seedCopy := make([]byte, len(seed)) copy(seedCopy, seed) //nolint:secmem-lint // required: ed25519.NewKeyFromSeed panics on mmap'd input, wiped via defer above defer secmem.SecureWipe(seedCopy) @@ -118,7 +129,23 @@ func (s *Ed25519Signer) MarshalOpenSSHPrivateKey(comment string) (*secmem.Secure } defer secmem.SecureWipe(block.Bytes) - pemBytes = pem.EncodeToMemory(block) + // pem.Encode into a pre-grown buffer rather than + // pem.EncodeToMemory. EncodeToMemory grows a bytes.Buffer as it + // writes, and every growth orphans the previous array — each one + // holding a prefix of the base64-encoded PRIVATE KEY, unreachable + // and unwiped. Sizing up front means one array, which the defer + // below wipes in full. + // + // The bound is deliberately loose: base64 expands by 4/3 plus a + // newline every 64 characters, so twice the input plus the header + // and footer cannot be reached, and Grow guarantees no reallocation + // below it. + var buf bytes.Buffer + buf.Grow(2*len(block.Bytes) + 128) + if err := pem.Encode(&buf, block); err != nil { + return fmt.Errorf("pem encode: %w", err) + } + pemBytes = buf.Bytes() return nil }) }) diff --git a/secret.go b/secret.go index 7fa2015..ebf9ac7 100644 --- a/secret.go +++ b/secret.go @@ -47,6 +47,8 @@ type Secret struct { // NewSecret copies b into hardened memory and returns the Secret. b is wiped // after the copy (defense-in-depth) — the caller must not reuse it. // +// As with [NewBuffer], b is wiped whether this call succeeds or fails. +// // Errors are those of [NewBuffer]: empty input, allocation or mlock failure, // and [ErrNoSecureMemory] on platforms without secure memory unless // [WithInsecureFallback] is passed. diff --git a/securebuf.go b/securebuf.go index e834737..e8c4b32 100644 --- a/securebuf.go +++ b/securebuf.go @@ -111,6 +111,16 @@ type SecureBuffer struct { // WARNING: raw is zeroed after copying. The caller must not reuse raw after // this call. If the same secret must be used multiple times, copy it first. // +// raw is zeroed whether this call SUCCEEDS OR FAILS. Wiping only on success +// would leave the caller's plaintext sitting in an ordinary heap slice that the +// warning above has just told them is gone — the worst of both, since they will +// not wipe it themselves. The only exception is an empty raw, where there is +// nothing to wipe. +// +// That means a retry after [ErrNoSecureMemory] has nothing left to copy. It does +// not need one: that error depends only on the platform and on +// [WithInsecureFallback], both knowable before the call — see [Probe]. +// // Common errors: EPERM / ENOMEM from mlock (RLIMIT_MEMLOCK exceeded — check // `ulimit -l` or systemd LimitMEMLOCK=). On platforms with no lockable // off-heap memory the error is [ErrNoSecureMemory] unless @@ -119,6 +129,10 @@ func NewBuffer(raw []byte, opts ...Option) (*SecureBuffer, error) { if len(raw) == 0 { return nil, errors.New("secmem.NewBuffer: empty input") } + // Deferred, not placed after the copy: every early return below is an error + // path that used to hand the caller back their plaintext intact. A defer + // also means a future error path cannot forget it. + defer secureWipeSlice(raw) if err := gateInsecure(platformHasSecureMemory, applyOptions(opts)); err != nil { return nil, fmt.Errorf("secmem.NewBuffer: %w", err) } @@ -131,7 +145,6 @@ func NewBuffer(raw []byte, opts ...Option) (*SecureBuffer, error) { return nil, fmt.Errorf("secmem.NewBuffer: %w", err) } copy(data, raw) - secureWipeSlice(raw) // zero the caller's copy defense-in-depth return newSecureBuffer(region, data, info), nil } @@ -163,6 +176,7 @@ func NewSyscallSafeBuffer(raw []byte, opts ...Option) (*SecureBuffer, error) { if len(raw) == 0 { return nil, errors.New("secmem.NewSyscallSafeBuffer: empty input") } + defer secureWipeSlice(raw) // on failure too — see NewBuffer if err := gateInsecure(platformHasSecureMemory, applyOptions(opts)); err != nil { return nil, fmt.Errorf("secmem.NewSyscallSafeBuffer: %w", err) } @@ -175,7 +189,6 @@ func NewSyscallSafeBuffer(raw []byte, opts ...Option) (*SecureBuffer, error) { return nil, fmt.Errorf("secmem.NewSyscallSafeBuffer: %w", err) } copy(data, raw) - secureWipeSlice(raw) return newSecureBuffer(region, data, info), nil } diff --git a/securebuf_test.go b/securebuf_test.go index 26f8277..34a94d3 100644 --- a/securebuf_test.go +++ b/securebuf_test.go @@ -1,6 +1,7 @@ package secmem import ( + "bytes" "errors" "runtime" "strconv" @@ -501,3 +502,36 @@ func janitorRegionCount() int { defer emergencyJanitor.mu.Unlock() return len(emergencyJanitor.regions) } + +// TestNewBuffer_WipesInputOnFailure pins the constructor contract on its error +// paths. The doc warns that raw is zeroed and must not be reused; wiping only on +// success left the caller's plaintext in an ordinary heap slice they had just +// been told was gone, so they would not wipe it themselves. +// +// Driven through an allocation the lock budget refuses, which is the only error +// path reachable without a platform that lacks secure memory: gateInsecure keys +// off a per-platform const, and fillCanary fails only if crypto/rand does. +// Inverting the repo's usual convention — an allocation that unexpectedly +// SUCCEEDS is the environment condition here, so that skips. +func TestNewBuffer_WipesInputOnFailure(t *testing.T) { + if !platformHasSecureMemory { + t.Skip("no secure memory on this platform") + } + const size = 512 << 20 // far past any default mlock / VirtualLock budget + raw := bytes.Repeat([]byte{0x5A}, size) + + buf, err := NewBuffer(raw) + if err == nil { + _ = buf.Destroy() + t.Skip("this environment locked 512 MiB; cannot exercise the failure path here") + } + if !bytes.Equal(raw, make([]byte, size)) { + nonzero := 0 + for _, b := range raw { + if b != 0 { + nonzero++ + } + } + t.Errorf("NewBuffer failed (%v) and left %d/%d plaintext bytes in the caller's slice", err, nonzero, size) + } +}