fix: wipe constructor inputs on failure; close three wipe-guarantee overclaims

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

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

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

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

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.

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
 }
 

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