KeychainData holds the age key passphrase as a plain string through JSON marshalling #36

New Issue

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

clawbot commented

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

From the 1.0 security survey. Confirmed still present.

Threat

internal/secret/keychainunlocker.go:49-53:

type KeychainData struct {
	AgePublicKey         string `json:"agePublicKey"`
	AgePrivKeyPassphrase string `json:"agePrivKeyPassphrase"`
	EncryptedLongtermKey string `json:"encryptedLongtermKey"`
}

AgePrivKeyPassphrase is the 64-hex-character passphrase protecting the keychain unlocker's age private key. It is handled as an ordinary Go string on both the create and the unlock path, which means it exists in unlocked, swappable, GC-managed heap in at least three simultaneous copies:

Create path: generateRandomPassphrase() (:368, backed by generateRandomString in internal/secret/helpers.go) returns a plain string. It is assigned into the struct at :421. json.Marshal at :425 produces a second unprotected copy as []byte. Only at :431 is anything wrapped in a LockedBuffer — by which point the plaintext has already been allocated twice in ordinary heap.
Unlock path: json.Unmarshal at :86 decodes into the same plain-string struct, and the wrap does not happen until :113.

Go strings are immutable and garbage-collected; there is no way to zero them, and encoding/json allocates its own internal buffers that the caller never sees and cannot wipe. So these copies persist in the heap until the allocator happens to reuse the pages, well past the point where the code believes it has cleaned up.

Recovering this passphrase from a swap file, hibernation image, or core dump — or by reading /proc/<pid>/mem as the same uid — yields the age private key for the keychain unlocker, and combined with the on-disk priv.age that is the vault's long-term key and therefore every secret in the vault. The rest of the codebase takes memguard seriously, which makes this path the weak link rather than a consistent level of protection.

Definition of done

The passphrase is never present in an unprotected string or []byte at any point in its lifetime, on either the create or the unlock path.
generateRandomPassphrase / generateRandomString return a *memguard.LockedBuffer rather than a string, or a locked-buffer variant is added and used here.
The keychain blob is serialized and deserialized without encoding/json ever holding the plaintext passphrase. Acceptable approaches: unmarshal into json.RawMessage and copy only the passphrase field into a locked buffer, or hand-roll the encode/decode for this struct. Whichever is chosen, the JSON wire format on disk must be unchanged so existing keychain items still load.
Backward compatibility is mandatory and must be tested. Existing users have keychain unlockers already stored. A round-trip test must prove that a blob written by the old code still decodes, and that a blob written by the new code is byte-identical in structure to what the old code produced.
All intermediate buffers are destroyed on every path including errors.
make check green. TODO.md updated in the same commit.

Implementation requirements

Verify the memguard version in go.mod before relying on LockedBuffer.String() semantics: in memguard v0.22 it returns a string aliasing the locked pages rather than copying, which is safe, but this must be confirmed rather than assumed. If it copies, that defeats the whole change.
Be careful that the fix does not merely move the copy. fmt.Sprintf, string concatenation, []byte(s) conversion, and passing a string to any stdlib function that buffers internally all reintroduce the problem.
This file is Darwin-gated and therefore not linted or exercised by the Linux CI runner. State explicitly in the PR how the change was verified, and do not assume a green CI means this code path was tested at all.
Do not fold in the other memguard gaps (the identity.String() sites, or GetSecretVersion returning plaintext). Those are tracked separately and this should stay reviewable on its own.

From the 1.0 security survey. Confirmed still present. ## Threat `internal/secret/keychainunlocker.go:49-53`: ```go type KeychainData struct { AgePublicKey string `json:"agePublicKey"` AgePrivKeyPassphrase string `json:"agePrivKeyPassphrase"` EncryptedLongtermKey string `json:"encryptedLongtermKey"` } ``` `AgePrivKeyPassphrase` is the 64-hex-character passphrase protecting the keychain unlocker's age private key. It is handled as an ordinary Go `string` on both the create and the unlock path, which means it exists in unlocked, swappable, GC-managed heap in **at least three simultaneous copies**: - **Create path:** `generateRandomPassphrase()` (`:368`, backed by `generateRandomString` in `internal/secret/helpers.go`) returns a plain `string`. It is assigned into the struct at `:421`. `json.Marshal` at `:425` produces a second unprotected copy as `[]byte`. Only at `:431` is anything wrapped in a `LockedBuffer` — by which point the plaintext has already been allocated twice in ordinary heap. - **Unlock path:** `json.Unmarshal` at `:86` decodes into the same plain-string struct, and the wrap does not happen until `:113`. Go strings are immutable and garbage-collected; there is no way to zero them, and `encoding/json` allocates its own internal buffers that the caller never sees and cannot wipe. So these copies persist in the heap until the allocator happens to reuse the pages, well past the point where the code believes it has cleaned up. Recovering this passphrase from a swap file, hibernation image, or core dump — or by reading `/proc/<pid>/mem` as the same uid — yields the age private key for the keychain unlocker, and combined with the on-disk `priv.age` that is the vault's long-term key and therefore every secret in the vault. The rest of the codebase takes memguard seriously, which makes this path the weak link rather than a consistent level of protection. ## Definition of done - The passphrase is never present in an unprotected `string` or `[]byte` at any point in its lifetime, on either the create or the unlock path. - `generateRandomPassphrase` / `generateRandomString` return a `*memguard.LockedBuffer` rather than a `string`, or a locked-buffer variant is added and used here. - The keychain blob is serialized and deserialized without `encoding/json` ever holding the plaintext passphrase. Acceptable approaches: unmarshal into `json.RawMessage` and copy only the passphrase field into a locked buffer, or hand-roll the encode/decode for this struct. Whichever is chosen, the JSON wire format on disk must be unchanged so existing keychain items still load. - **Backward compatibility is mandatory and must be tested.** Existing users have keychain unlockers already stored. A round-trip test must prove that a blob written by the old code still decodes, and that a blob written by the new code is byte-identical in structure to what the old code produced. - All intermediate buffers are destroyed on every path including errors. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Verify the memguard version in `go.mod` before relying on `LockedBuffer.String()` semantics: in memguard v0.22 it returns a string aliasing the locked pages rather than copying, which is safe, but this must be confirmed rather than assumed. If it copies, that defeats the whole change. - Be careful that the fix does not merely move the copy. `fmt.Sprintf`, string concatenation, `[]byte(s)` conversion, and passing a string to any stdlib function that buffers internally all reintroduce the problem. - This file is Darwin-gated and therefore **not linted or exercised by the Linux CI runner**. State explicitly in the PR how the change was verified, and do not assume a green CI means this code path was tested at all. - Do not fold in the other memguard gaps (the `identity.String()` sites, or `GetSecretVersion` returning plaintext). Those are tracked separately and this should stay reviewable on its own.

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

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.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/secret#36