memguard lifecycle is not wired up: Ctrl-C and error exits leave key material in locked memory unwiped #35

New Issue

clawbot · 2026-08-09T03:39:22+02:00

clawbot commented

2026-08-09 03:39:22 +02:00

From the 1.0 security survey. Highest value-per-line item on the milestone: roughly five lines of code closes it.

Threat

The repo uses memguard throughout, and per-allocation mlock is therefore already in effect for every LockedBuffer. What is missing is the process-level lifecycle that memguard requires to actually guarantee wiping.

cmd/secret/main.go is eight lines and calls cli.Entry(). internal/cli/root.go:19-26:

func Entry() {
    if err := cmd.Execute(); err != nil {
        os.Exit(1)
    }
}

There is no memguard.CatchInterrupt() and no defer memguard.Purge() anywhere in the repository. Three consequences, all of which leave plaintext key material in locked pages:

Signals. Ctrl-C or SIGTERM during any operation terminates the process immediately. Every live LockedBuffer — which at various moments holds the vault long-term private key, an unlocker passphrase, a version private key, or a decrypted secret value — is never destroyed. memguard.CatchInterrupt() exists precisely to install a handler that purges before exiting; without it the mlocked pages are released to the kernel unwiped. Interrupting a secret get that is waiting on a Touch ID prompt or a GPG pinentry is an entirely ordinary user action.
os.Exit(1) on the error path skips every deferred Destroy(). Go does not run deferred functions on os.Exit. The codebase relies on defer buf.Destroy() as its wiping mechanism, so every failure that propagates an error up to Entry() — a wrong passphrase, a missing unlocker, an I/O error mid-decrypt — exits with buffers intact. The error paths are exactly the paths where a partially-decrypted key is most likely to be sitting in memory.
No final Purge() on the normal exit path either, so the guarantee depends entirely on every individual defer being present and correct on every path.

The exposure is the standard one for unwiped key material: swap, hibernation images, core dumps, and /proc/<pid>/mem for same-uid processes. It is meaningfully worse here than in a typical program because the buffers hold the long-term vault key, from which every secret in the vault is recoverable, not a single session credential.

Note that CI already runs with --ulimit memlock=-1:-1 (script/cibuild) because the suite exercises memguard, so the locking half of this is real and working. Only the teardown is absent.

Definition of done

memguard.CatchInterrupt() is called once, early, before any secret-bearing work can begin.
defer memguard.Purge() covers the full program lifetime.
No code path reachable after a LockedBuffer may exist calls os.Exit without purging first. The current os.Exit(1) in Entry() must either be preceded by an explicit memguard.Purge() or restructured so the deferred purge runs — the usual shape is an inner function that returns an exit code, with os.Exit called only from the outermost frame after all defers have run.
Audit for any other os.Exit, log.Fatal, log.Fatalf, or panic in non-test code that could bypass the purge, and fix or justify each. log.Fatal calls os.Exit and is just as skip-prone.
A test proves the wiring exists — at minimum that Entry (or the extracted inner function) returns rather than exiting, so the deferred purge is reachable. Signal handling is awkward to test directly; if it cannot be tested reliably, say so on the issue rather than adding a flaky test.
make check green. TODO.md updated in the same commit.

Implementation requirements

memguard.CatchInterrupt() installs a signal handler that terminates the process. Verify this does not break the interactive prompts — the passphrase and mnemonic readers put the terminal in a raw or no-echo mode, and a handler that exits without restoring it leaves the user's terminal with echo disabled. If that is a real risk, restore terminal state in the purge path and note it in the commit message.
Do not scatter memguard.Purge() through the codebase. One place, at the top level. Purge is global and idempotent; calling it from many sites invites destroying buffers another frame still holds.
Keep the change to cmd/secret/main.go and internal/cli/root.go. Do not fold in unrelated memory hardening — the other memguard gaps are tracked separately, and this one should stay small enough to review at a glance.

From the 1.0 security survey. Highest value-per-line item on the milestone: roughly five lines of code closes it. ## Threat The repo uses `memguard` throughout, and per-allocation `mlock` is therefore already in effect for every `LockedBuffer`. What is missing is the process-level lifecycle that memguard requires to actually guarantee wiping. `cmd/secret/main.go` is eight lines and calls `cli.Entry()`. `internal/cli/root.go:19-26`: ```go func Entry() { if err := cmd.Execute(); err != nil { os.Exit(1) } } ``` There is **no `memguard.CatchInterrupt()`** and **no `defer memguard.Purge()`** anywhere in the repository. Three consequences, all of which leave plaintext key material in locked pages: 1. **Signals.** Ctrl-C or `SIGTERM` during any operation terminates the process immediately. Every live `LockedBuffer` — which at various moments holds the vault long-term private key, an unlocker passphrase, a version private key, or a decrypted secret value — is never destroyed. `memguard.CatchInterrupt()` exists precisely to install a handler that purges before exiting; without it the mlocked pages are released to the kernel unwiped. Interrupting a `secret get` that is waiting on a Touch ID prompt or a GPG pinentry is an entirely ordinary user action. 2. **`os.Exit(1)` on the error path skips every deferred `Destroy()`.** Go does not run deferred functions on `os.Exit`. The codebase relies on `defer buf.Destroy()` as its wiping mechanism, so every failure that propagates an error up to `Entry()` — a wrong passphrase, a missing unlocker, an I/O error mid-decrypt — exits with buffers intact. The error paths are exactly the paths where a partially-decrypted key is most likely to be sitting in memory. 3. **No final `Purge()`** on the normal exit path either, so the guarantee depends entirely on every individual `defer` being present and correct on every path. The exposure is the standard one for unwiped key material: swap, hibernation images, core dumps, and `/proc/<pid>/mem` for same-uid processes. It is meaningfully worse here than in a typical program because the buffers hold the *long-term vault key*, from which every secret in the vault is recoverable, not a single session credential. Note that CI already runs with `--ulimit memlock=-1:-1` (`script/cibuild`) because the suite exercises memguard, so the locking half of this is real and working. Only the teardown is absent. ## Definition of done - `memguard.CatchInterrupt()` is called once, early, before any secret-bearing work can begin. - `defer memguard.Purge()` covers the full program lifetime. - **No code path reachable after a `LockedBuffer` may exist calls `os.Exit` without purging first.** The current `os.Exit(1)` in `Entry()` must either be preceded by an explicit `memguard.Purge()` or restructured so the deferred purge runs — the usual shape is an inner function that returns an exit code, with `os.Exit` called only from the outermost frame after all defers have run. - Audit for any other `os.Exit`, `log.Fatal`, `log.Fatalf`, or `panic` in non-test code that could bypass the purge, and fix or justify each. `log.Fatal` calls `os.Exit` and is just as skip-prone. - A test proves the wiring exists — at minimum that `Entry` (or the extracted inner function) returns rather than exiting, so the deferred purge is reachable. Signal handling is awkward to test directly; if it cannot be tested reliably, say so on the issue rather than adding a flaky test. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - `memguard.CatchInterrupt()` installs a signal handler that terminates the process. Verify this does not break the interactive prompts — the passphrase and mnemonic readers put the terminal in a raw or no-echo mode, and a handler that exits without restoring it leaves the user's terminal with echo disabled. If that is a real risk, restore terminal state in the purge path and note it in the commit message. - Do not scatter `memguard.Purge()` through the codebase. One place, at the top level. Purge is global and idempotent; calling it from many sites invites destroying buffers another frame still holds. - Keep the change to `cmd/secret/main.go` and `internal/cli/root.go`. Do not fold in unrelated memory hardening — the other memguard gaps are tracked separately, and this one should stay small enough to review at a glance.

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

clawbot referenced this issue

2026-08-09 03:41:34 +02:00

Cobra prints full usage on internal failures, burying the actual error #41

clawbot referenced this issue

2026-08-09 03:41:58 +02:00

PGPUnlocker.GetID panics on corrupt metadata, and setMnemonicEnv re-exports the mnemonic #42

Sign in to join this conversation.