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

New Issue

clawbot · 2026-08-09T03:41:58+02:00

clawbot commented

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

Two related robustness and exposure defects from the 1.0 security survey. Both are small and both are in the "a bad state gets worse" category.

1. Panic on corrupt metadata

internal/secret/pgpunlocker.go:159-169 — GetID() calls panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", err)).

The comment at :163-164 argues the panic is intentional, on the grounds that a fallback ID would mask data corruption. The goal is right; the mechanism is wrong. GetID() is called from listing paths, so a single corrupt unlocker directory takes down secret unlocker list entirely — the user cannot even enumerate their unlockers to discover which one is broken, let alone remove it. A panic also prints a Go stack trace, which is not a usable error message, and it bypasses every deferred LockedBuffer.Destroy() in the process (see #35).

The equivalent code in keychainunlocker.go was already fixed: GetID() (:163-176) no longer reads metadata and degrades to "unknown". This one was missed.

Worth noting the precedent: #1 in this tracker was the same class of bug — a single unlocker with missing metadata broke the list command — and was fixed by skipping the bad directory with a warning. That is the established pattern in this repo.

The only other panic in non-test code is pkg/bip85/bip85.go:87, which guards a genuine internal invariant and is fine.

2. `setMnemonicEnv` puts the mnemonic back into the environment

internal/cli/vault.go:235 sets the mnemonic into the process environment for the duration of vault creation, with a restore closure that unsets it at :241. That os.Unsetenv is the only one in non-test code, and the net effect of the helper is to increase exposure, not reduce it: for the duration of the call the mnemonic — the root secret from which every vault key derives — is readable in /proc/<pid>/environ by any same-uid process and is inherited by every child process spawned in that window, which includes gpg.

More broadly, SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE are read in a dozen places and never cleared. The README (:81-82, :305-309) documents them as conveniences that avoid the interactive prompt, with no warning that they are visible to same-uid processes, inherited by children, and routinely captured in shell history and CI logs.

Definition of done

GetID() returns an error, or degrades to a clearly-marked placeholder the way the keychain unlocker does, instead of panicking. Whichever is chosen, a corrupt unlocker directory must not prevent secret unlocker list from listing the others.
A corrupt or unreadable unlocker is surfaced to the user via the existing secret.Warn() mechanism, naming the directory, so it is diagnosable and removable.
setMnemonicEnv no longer round-trips the mnemonic through the process environment. Pass it in memory instead — a *memguard.LockedBuffer argument through the call chain.
Environment variables holding secrets are unset immediately after being read into a locked buffer, at every read site.
README gains an explicit warning under Environment Variables that these variables are visible to other processes running as the same user, are inherited by child processes including gpg, and end up in shell history and CI logs — with the interactive prompt named as the safer default.
Tests: a corrupt unlocker metadata file does not panic and does not break listing; the mnemonic is absent from the environment after vault creation.
make check green. TODO.md updated in the same commit.

Implementation requirements

Unsetting an environment variable does not reliably erase it from the process's memory — the C environ block may retain the bytes. Unset it anyway, because it stops inheritance and /proc exposure, which is the dominant risk, but do not describe it in comments or docs as if it wipes the value.
Check whether anything depends on the mnemonic being present in the environment for a subprocess. If a child process genuinely needs it, say so on this issue before changing the mechanism.
These two items are grouped because both are small and both touch the same "handle a bad state without making it worse" theme. If the implementer finds either grows beyond a tight diff, split it and say so rather than landing one oversized commit.

Two related robustness and exposure defects from the 1.0 security survey. Both are small and both are in the "a bad state gets worse" category. ## 1. Panic on corrupt metadata `internal/secret/pgpunlocker.go:159-169` — `GetID()` calls `panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", err))`. The comment at `:163-164` argues the panic is intentional, on the grounds that a fallback ID would mask data corruption. The goal is right; the mechanism is wrong. `GetID()` is called from listing paths, so a single corrupt unlocker directory takes down `secret unlocker list` entirely — the user cannot even enumerate their unlockers to discover which one is broken, let alone remove it. A panic also prints a Go stack trace, which is not a usable error message, and it bypasses every deferred `LockedBuffer.Destroy()` in the process (see #35). The equivalent code in `keychainunlocker.go` was already fixed: `GetID()` (`:163-176`) no longer reads metadata and degrades to `"unknown"`. This one was missed. Worth noting the precedent: #1 in this tracker was the same class of bug — a single unlocker with missing metadata broke the `list` command — and was fixed by skipping the bad directory with a warning. That is the established pattern in this repo. The only other panic in non-test code is `pkg/bip85/bip85.go:87`, which guards a genuine internal invariant and is fine. ## 2. `setMnemonicEnv` puts the mnemonic back into the environment `internal/cli/vault.go:235` sets the mnemonic into the process environment for the duration of vault creation, with a restore closure that unsets it at `:241`. That `os.Unsetenv` is the only one in non-test code, and the net effect of the helper is to **increase** exposure, not reduce it: for the duration of the call the mnemonic — the root secret from which every vault key derives — is readable in `/proc/<pid>/environ` by any same-uid process and is inherited by every child process spawned in that window, which includes `gpg`. More broadly, `SB_SECRET_MNEMONIC` and `SB_UNLOCK_PASSPHRASE` are read in a dozen places and never cleared. The README (`:81-82`, `:305-309`) documents them as conveniences that avoid the interactive prompt, with no warning that they are visible to same-uid processes, inherited by children, and routinely captured in shell history and CI logs. ## Definition of done - `GetID()` returns an error, or degrades to a clearly-marked placeholder the way the keychain unlocker does, instead of panicking. Whichever is chosen, a corrupt unlocker directory must not prevent `secret unlocker list` from listing the others. - A corrupt or unreadable unlocker is surfaced to the user via the existing `secret.Warn()` mechanism, naming the directory, so it is diagnosable and removable. - `setMnemonicEnv` no longer round-trips the mnemonic through the process environment. Pass it in memory instead — a `*memguard.LockedBuffer` argument through the call chain. - Environment variables holding secrets are unset immediately after being read into a locked buffer, at every read site. - README gains an explicit warning under Environment Variables that these variables are visible to other processes running as the same user, are inherited by child processes including `gpg`, and end up in shell history and CI logs — with the interactive prompt named as the safer default. - Tests: a corrupt unlocker metadata file does not panic and does not break listing; the mnemonic is absent from the environment after vault creation. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Unsetting an environment variable does not reliably erase it from the process's memory — the C environ block may retain the bytes. Unset it anyway, because it stops *inheritance and `/proc` exposure*, which is the dominant risk, but do not describe it in comments or docs as if it wipes the value. - Check whether anything depends on the mnemonic being present in the environment for a subprocess. If a child process genuinely needs it, say so on this issue before changing the mechanism. - These two items are grouped because both are small and both touch the same "handle a bad state without making it worse" theme. If the implementer finds either grows beyond a tight diff, split it and say so rather than landing one oversized commit.

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

clawbot referenced this issue

2026-08-09 03:44:00 +02:00

CreatePGPUnlocker leaves a partial unlocker directory on disk when fingerprint resolution fails #48

clawbot referenced this issue

2026-08-09 03:46:49 +02:00

Darwin-only code is never compiled, linted, or tested by CI, including both hardware-backed unlockers #50

Sign in to join this conversation.