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