age identity .String() creates unprotected copies of private keys at six call sites #38

New Issue

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

clawbot commented

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

From the 1.0 security survey. Confirmed still present, with one part of the original TODO claim now stale.

Threat

age.X25519Identity.String() returns a plain Go string containing the bech32-encoded X25519 private key. At each site below the code calls .String() and only afterwards wraps the result in a LockedBuffer — so an unprotected, immutable, unzeroable copy of the private key is allocated in ordinary heap first, and a second copy is created by the []byte(...) conversion:

internal/secret/keychainunlocker.go:278 — memguard.NewBufferFromBytes([]byte(ltIdentity.String())) — this one is the vault long-term key
internal/secret/keychainunlocker.go:382-383 — agePrivKeyStr := ageIdentity.String() then wrap
internal/secret/pgpunlocker.go:308 — memguard.NewBufferFromBytes([]byte(ageIdentity.String()))
internal/vault/unlockers.go:401 and :478 — privKeyStr := unlockerIdentity.String()
internal/secret/version.go:167-168 — version keypair private key
internal/secret/seunlocker_darwin.go:347

Several of these are the long-term vault key, from which every secret in the vault is derivable. Go strings cannot be zeroed and are garbage-collected, so these copies persist in the heap past the point the code believes it has cleaned up, exposed via swap, hibernation images, core dumps, and same-uid /proc/<pid>/mem reads.

Correction to the old TODO entry: it listed internal/cli/version.go as one of these sites. That is stale — that file contains no .String() call on an identity. internal/cli/crypto.go is also largely fixed: the key is held in a LockedBuffer throughout, with the one genuine residual being crypto.go:95, memguard.NewBufferFromBytes([]byte(identity.String())), which belongs on this list. The other .String() calls in that file (:114, :139, :220, :225) are LockedBuffer.String(), which in memguard v0.22 aliases the locked pages rather than copying, and are therefore materially lower risk.

Definition of done

A single shared helper — something like secret.identityToLockedBuffer(id) — performs the identity-to-locked-buffer conversion in exactly one place.
All seven sites listed above (the six plus internal/cli/crypto.go:95) use it. No []byte(identity.String()) pattern remains anywhere in non-test code; a grep for it in the PR must come back empty.
The helper makes a best-effort attempt to minimize and zero the intermediate. Because age's API returns a string by value, a perfect fix is not reachable without reimplementing bech32 encoding directly into locked memory. A best-effort helper with the limitation documented in a comment is acceptable and is the expected outcome — but the limitation must be written down at the helper, not left implicit.
If the implementer concludes that reimplementing bech32 into a locked buffer is worth doing, that is a larger change: say so on this issue and get agreement before writing it, rather than expanding scope unilaterally.
Tests confirm every affected unlocker type still round-trips: create an unlocker, then unlock with it, for passphrase and PGP at minimum.
make check green. TODO.md updated in the same commit, including correcting the stale internal/cli/version.go reference.

Implementation requirements

Confirm the memguard version in go.mod and the actual semantics of LockedBuffer.String() before relying on the aliasing behavior described above. If it copies rather than aliases, the four internal/cli/crypto.go sites become real findings and must be handled too.
Two of these files (keychainunlocker.go, seunlocker_darwin.go) are Darwin-gated and are not compiled or linted by the Linux CI runner. A green CI proves nothing about them. State in the PR how those two were verified.
Do not change the on-disk format. These are all in-memory representation changes.
Do not fold in the KeychainData plain-string passphrase or the GetSecretVersion plaintext copy; both are tracked separately.

From the 1.0 security survey. Confirmed still present, with one part of the original TODO claim now stale. ## Threat `age.X25519Identity.String()` returns a plain Go `string` containing the bech32-encoded X25519 **private key**. At each site below the code calls `.String()` and only afterwards wraps the result in a `LockedBuffer` — so an unprotected, immutable, unzeroable copy of the private key is allocated in ordinary heap first, and a second copy is created by the `[]byte(...)` conversion: - `internal/secret/keychainunlocker.go:278` — `memguard.NewBufferFromBytes([]byte(ltIdentity.String()))` — this one is the **vault long-term key** - `internal/secret/keychainunlocker.go:382-383` — `agePrivKeyStr := ageIdentity.String()` then wrap - `internal/secret/pgpunlocker.go:308` — `memguard.NewBufferFromBytes([]byte(ageIdentity.String()))` - `internal/vault/unlockers.go:401` and `:478` — `privKeyStr := unlockerIdentity.String()` - `internal/secret/version.go:167-168` — version keypair private key - `internal/secret/seunlocker_darwin.go:347` Several of these are the long-term vault key, from which every secret in the vault is derivable. Go strings cannot be zeroed and are garbage-collected, so these copies persist in the heap past the point the code believes it has cleaned up, exposed via swap, hibernation images, core dumps, and same-uid `/proc/<pid>/mem` reads. **Correction to the old TODO entry:** it listed `internal/cli/version.go` as one of these sites. That is stale — that file contains no `.String()` call on an identity. `internal/cli/crypto.go` is also largely fixed: the key is held in a `LockedBuffer` throughout, with the one genuine residual being `crypto.go:95`, `memguard.NewBufferFromBytes([]byte(identity.String()))`, which belongs on this list. The other `.String()` calls in that file (`:114`, `:139`, `:220`, `:225`) are `LockedBuffer.String()`, which in memguard v0.22 aliases the locked pages rather than copying, and are therefore materially lower risk. ## Definition of done - A single shared helper — something like `secret.identityToLockedBuffer(id)` — performs the identity-to-locked-buffer conversion in exactly one place. - All seven sites listed above (the six plus `internal/cli/crypto.go:95`) use it. No `[]byte(identity.String())` pattern remains anywhere in non-test code; a grep for it in the PR must come back empty. - The helper makes a best-effort attempt to minimize and zero the intermediate. Because `age`'s API returns a `string` by value, a perfect fix is not reachable without reimplementing bech32 encoding directly into locked memory. **A best-effort helper with the limitation documented in a comment is acceptable and is the expected outcome** — but the limitation must be written down at the helper, not left implicit. - If the implementer concludes that reimplementing bech32 into a locked buffer is worth doing, that is a larger change: say so on this issue and get agreement before writing it, rather than expanding scope unilaterally. - Tests confirm every affected unlocker type still round-trips: create an unlocker, then unlock with it, for passphrase and PGP at minimum. - `make check` green. `TODO.md` updated in the same commit, including correcting the stale `internal/cli/version.go` reference. ## Implementation requirements - Confirm the memguard version in `go.mod` and the actual semantics of `LockedBuffer.String()` before relying on the aliasing behavior described above. If it copies rather than aliases, the four `internal/cli/crypto.go` sites become real findings and must be handled too. - Two of these files (`keychainunlocker.go`, `seunlocker_darwin.go`) are Darwin-gated and are **not compiled or linted by the Linux CI runner**. A green CI proves nothing about them. State in the PR how those two were verified. - Do not change the on-disk format. These are all in-memory representation changes. - Do not fold in the `KeychainData` plain-string passphrase or the `GetSecretVersion` plaintext copy; both are tracked separately.

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

clawbot referenced this issue

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

README missing four required sections; TODO.md is stale and describes completed work as pending #46

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.