GetSecretVersion copies decrypted secrets out of locked memory into plain []byte #37

New Issue

clawbot · 2026-08-09T03:40:04+02:00

clawbot commented

2026-08-09 03:40:04 +02:00

From the 1.0 security survey. The largest remaining memory-hygiene hole, because it is on the hot path of the tool's most-used command.

Threat

internal/vault/secrets.go:303-305:

result := make([]byte, decryptedValue.Size())
copy(result, decryptedValue.Bytes())
decryptedValue.Destroy()

The value is decrypted into a memguard.LockedBuffer, then deliberately copied out into ordinary heap and the locked buffer destroyed. The returned []byte is unlocked, swappable, never zeroed, and garbage-collected. Every single secret get therefore ends with the full plaintext secret sitting in unprotected memory for the remainder of the process lifetime and beyond.

Destroying the locked buffer immediately after copying makes this worse rather than better: it gives the appearance of careful cleanup while the plaintext has just been moved somewhere that cannot be cleaned up at all.

The codebase already knows the correct pattern — DecryptWithIdentity (internal/secret/crypto.go:96-100) zeroes its scratch slice after use. internal/vault/secrets.go simply does not follow it.

Exposure is the usual set: swap, hibernation, core dumps, /proc/<pid>/mem for a same-uid process. The distinguishing factor here is reach: this is not a key that protects data, it is the data, and it applies to every secret the user has ever read. Combined with the missing memguard.Purge() on exit paths (tracked separately), a secret get followed by an error exit leaves the plaintext in memory with nothing having wiped it.

Definition of done

Vault.GetSecret and Vault.GetSecretVersion return a *memguard.LockedBuffer rather than []byte, so the plaintext never leaves locked memory inside the library.
All callers in internal/cli are updated — roughly three to four call sites — and each destroys the buffer on every path, including error paths.
The final write to stdout is the only point where plaintext leaves a locked buffer, and it writes directly from the buffer without an intervening string or []byte copy.
No caller retains the buffer past its useful life, and none returns it further up without a matching destroy.
Tests confirm the round-trip still produces the correct value for both the current version and an explicit --version, and that binary secrets containing NUL bytes and non-UTF-8 sequences survive intact — a change of this shape is exactly where a string conversion silently mangles binary data.
make check green. TODO.md updated in the same commit.

Implementation requirements

This is an API change to an exported function. Check for other callers before assuming there are only the CLI ones.
Do not paper over it by keeping the []byte signature and zeroing the slice in the caller. Callers forget, and the whole point is to make the unsafe thing impossible rather than merely discouraged.
Writing to stdout: prefer writing the buffer's bytes directly to the io.Writer. Avoid fmt.Println, fmt.Fprintf("%s"), and any path that converts to string first, each of which allocates an unprotected copy and reintroduces the bug at the last step.
Watch the trailing-newline behavior of secret get carefully — it is user-visible and scripts depend on it. Whatever the current behavior is, preserve it exactly, and add a test that pins it if none exists.
Sequence this after the lint branch lands; internal/vault/secrets.go is heavily rewritten there.

From the 1.0 security survey. The largest remaining memory-hygiene hole, because it is on the hot path of the tool's most-used command. ## Threat `internal/vault/secrets.go:303-305`: ```go result := make([]byte, decryptedValue.Size()) copy(result, decryptedValue.Bytes()) decryptedValue.Destroy() ``` The value is decrypted into a `memguard.LockedBuffer`, then **deliberately copied out into ordinary heap** and the locked buffer destroyed. The returned `[]byte` is unlocked, swappable, never zeroed, and garbage-collected. Every single `secret get` therefore ends with the full plaintext secret sitting in unprotected memory for the remainder of the process lifetime and beyond. Destroying the locked buffer immediately after copying makes this worse rather than better: it gives the appearance of careful cleanup while the plaintext has just been moved somewhere that cannot be cleaned up at all. The codebase already knows the correct pattern — `DecryptWithIdentity` (`internal/secret/crypto.go:96-100`) zeroes its scratch slice after use. `internal/vault/secrets.go` simply does not follow it. Exposure is the usual set: swap, hibernation, core dumps, `/proc/<pid>/mem` for a same-uid process. The distinguishing factor here is reach: this is not a key that protects data, it *is* the data, and it applies to every secret the user has ever read. Combined with the missing `memguard.Purge()` on exit paths (tracked separately), a `secret get` followed by an error exit leaves the plaintext in memory with nothing having wiped it. ## Definition of done - `Vault.GetSecret` and `Vault.GetSecretVersion` return a `*memguard.LockedBuffer` rather than `[]byte`, so the plaintext never leaves locked memory inside the library. - All callers in `internal/cli` are updated — roughly three to four call sites — and each destroys the buffer on every path, including error paths. - The final write to stdout is the only point where plaintext leaves a locked buffer, and it writes directly from the buffer without an intervening `string` or `[]byte` copy. - No caller retains the buffer past its useful life, and none returns it further up without a matching destroy. - Tests confirm the round-trip still produces the correct value for both the current version and an explicit `--version`, and that binary secrets containing NUL bytes and non-UTF-8 sequences survive intact — a change of this shape is exactly where a `string` conversion silently mangles binary data. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - This is an API change to an exported function. Check for other callers before assuming there are only the CLI ones. - Do not paper over it by keeping the `[]byte` signature and zeroing the slice in the caller. Callers forget, and the whole point is to make the unsafe thing impossible rather than merely discouraged. - Writing to stdout: prefer writing the buffer's bytes directly to the `io.Writer`. Avoid `fmt.Println`, `fmt.Fprintf("%s")`, and any path that converts to `string` first, each of which allocates an unprotected copy and reintroduces the bug at the last step. - Watch the trailing-newline behavior of `secret get` carefully — it is user-visible and scripts depend on it. Whatever the current behavior is, preserve it exactly, and add a test that pins it if none exists. - Sequence this after the lint branch lands; `internal/vault/secrets.go` is heavily rewritten there.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:40:04 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/secret#37