From c2b3ac03b41d737b3c80f092d68b312f93fcd58e Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 3 Oct 2026 12:17:24 +0000 Subject: [PATCH] Keep the keychain unlocker passphrase in locked memory (closes #36) The passphrase protecting the keychain unlocker's age key was a plain string passed through encoding/json, leaving copies in ordinary memory when an unlocker was created and each time one was used. It is now generated into a locked buffer, and KeychainData, moved to keychaindata.go, which is not darwin-only so its tests run on Linux, writes and reads the keychain JSON itself: encode copies the parts straight into a locked buffer, and decodeKeychainData takes the passphrase from a json.RawMessage that it wipes. The JSON field names are unchanged. keychainunlocker.go only calls this code and stores the item from the locked buffer without a string copy. Model: opus-5-5 --- TODO.md | 16 +-- internal/secret/helpers_darwin.go | 29 ------ internal/secret/keychaindata.go | 142 +++++++++++++++++++++++++++ internal/secret/keychaindata_test.go | 118 ++++++++++++++++++++++ internal/secret/keychainunlocker.go | 44 +++------ 5 files changed, 284 insertions(+), 65 deletions(-) delete mode 100644 internal/secret/helpers_darwin.go create mode 100644 internal/secret/keychaindata.go create mode 100644 internal/secret/keychaindata_test.go diff --git a/TODO.md b/TODO.md index 281cbac..38d9581 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,11 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-03: The keychain unlocker's age key passphrase stays in + locked memory: it is generated into a locked buffer, and the + keychain JSON is written and read by `KeychainData` code in + `internal/secret/keychaindata.go` (tested on Linux) without + `encoding/json` holding it; the JSON field names are unchanged. - 2026-10-02: A plain `docker build .` builds again: the size tests skip a case that needs more locked memory than the process can lock, and run every case under `script/cibuild`. The image stamps the @@ -88,12 +93,11 @@ Bring the repo into policy compliance in one commit: - Command injection: GPG key IDs passed unescaped to exec.Command (pgpunlocker.go:323-327); data.String() passed unescaped to the security command (keychainunlocker.go:472-476). - - Memory security: KeychainData stores AgePrivKeyPassphrase as a - plain string (keychainunlocker.go:342,393-396); age identity - .String() creates unprotected copies (keychainunlocker.go:356, - pgpunlocker.go:256, version.go:155); age secret key held in a - plain string in cli/crypto.go:86,91,113; private keys exposed via - buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase. + - Memory security: age identity .String() creates unprotected + copies (keychainunlocker.go:356, pgpunlocker.go:256, + version.go:155); age secret key held in a plain string in + cli/crypto.go:86,91,113; private keys exposed via buffer.Bytes() + to GPGEncryptFunc and EncryptWithPassphrase. - Race conditions: no file locking in vault/secrets.go:142-176; non-atomic writes can leave the vault inconsistent. - Input validation: dots in secret names risk path traversal diff --git a/internal/secret/helpers_darwin.go b/internal/secret/helpers_darwin.go deleted file mode 100644 index 435e665..0000000 --- a/internal/secret/helpers_darwin.go +++ /dev/null @@ -1,29 +0,0 @@ -//go:build darwin - -package secret - -import ( - "crypto/rand" - "fmt" - "math/big" -) - -// generateRandomString generates a random string of the specified length using the given character set -func generateRandomString(length int, charset string) (string, error) { - if length <= 0 { - return "", fmt.Errorf("length must be positive") - } - - result := make([]byte, length) - charsetLen := big.NewInt(int64(len(charset))) - - for i := range length { - randomIndex, err := rand.Int(rand.Reader, charsetLen) - if err != nil { - return "", fmt.Errorf("failed to generate random number: %w", err) - } - result[i] = charset[randomIndex.Int64()] - } - - return string(result), nil -} diff --git a/internal/secret/keychaindata.go b/internal/secret/keychaindata.go new file mode 100644 index 0000000..5d5d388 --- /dev/null +++ b/internal/secret/keychaindata.go @@ -0,0 +1,142 @@ +package secret + +import ( + "bytes" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "strings" + + "github.com/awnumar/memguard" +) + +var ( + errPassphraseLength = errors.New( + "passphrase length must be a positive even number") + errPassphraseNotHex = errors.New( + "keychain passphrase must be lowercase hex") + errNoKeychainPassphrase = errors.New( + "keychain data has no agePrivKeyPassphrase string") +) + +// KeychainData is what a keychain unlocker stores in the macOS keychain. +// It is stored as JSON, but encode and decodeKeychainData keep the +// passphrase out of encoding/json, which would leave copies of it in +// ordinary memory. +type KeychainData struct { + AgePublicKey string + AgePrivKeyPassphrase *memguard.LockedBuffer + EncryptedLongtermKey string +} + +// generateRandomPassphrase returns length random lowercase hex characters +// in a locked buffer. The caller must destroy it. +func generateRandomPassphrase(length int) (*memguard.LockedBuffer, error) { + // Each random byte becomes two hex characters. + randomBytes := hex.DecodedLen(length) + if length <= 0 || hex.EncodedLen(randomBytes) != length { + return nil, errPassphraseLength + } + + random := memguard.NewBufferRandom(randomBytes) + defer random.Destroy() + + passphrase := memguard.NewBuffer(length) + hex.Encode(passphrase.Bytes(), random.Bytes()) + passphrase.Freeze() + + return passphrase, nil +} + +// encode returns d as JSON in a locked buffer: +// {"agePublicKey":"...","agePrivKeyPassphrase":"...","encryptedLongtermKey":"..."}. +// The passphrase is copied straight into the buffer, so it must be hex, +// which JSON does not escape. The caller must destroy the returned buffer. +func (d *KeychainData) encode() (*memguard.LockedBuffer, error) { + if d.AgePrivKeyPassphrase == nil { + return nil, errNilPassphraseBuffer + } + + if d.AgePrivKeyPassphrase.Size() == 0 { + return nil, errEmptyPassphrase + } + + for _, c := range d.AgePrivKeyPassphrase.Bytes() { + if strings.IndexByte("0123456789abcdef", c) < 0 { + return nil, errPassphraseNotHex + } + } + + publicKey, err := json.Marshal(d.AgePublicKey) + if err != nil { + return nil, fmt.Errorf("failed to encode age public key: %w", err) + } + + longtermKey, err := json.Marshal(d.EncryptedLongtermKey) + if err != nil { + return nil, fmt.Errorf("failed to encode long-term key: %w", err) + } + + parts := [][]byte{ + []byte(`{"agePublicKey":`), publicKey, + []byte(`,"agePrivKeyPassphrase":"`), d.AgePrivKeyPassphrase.Bytes(), + []byte(`","encryptedLongtermKey":`), longtermKey, + []byte(`}`), + } + + size := 0 + for _, part := range parts { + size += len(part) + } + + encoded := memguard.NewBuffer(size) + + written := 0 + for _, part := range parts { + written += copy(encoded.Bytes()[written:], part) + } + + encoded.Freeze() + + return encoded, nil +} + +// decodeKeychainData parses keychain data written by encode. The caller +// must destroy the returned AgePrivKeyPassphrase. +func decodeKeychainData(data *memguard.LockedBuffer) (*KeychainData, error) { + if data == nil { + return nil, errNilDataBuffer + } + + // json.Unmarshal gives a json.RawMessage field the field's JSON text + // unchanged, in the one copy RawMessage makes; it is wiped on return. + var fields struct { + AgePublicKey string `json:"agePublicKey"` + AgePrivKeyPassphrase json.RawMessage `json:"agePrivKeyPassphrase"` + EncryptedLongtermKey string `json:"encryptedLongtermKey"` + } + + defer func() { memguard.WipeBytes(fields.AgePrivKeyPassphrase) }() + + err := json.Unmarshal(data.Bytes(), &fields) + if err != nil { + return nil, fmt.Errorf("failed to parse keychain data: %w", err) + } + + // json.Unmarshal accepted the JSON, so text that starts with a quote is + // a whole string. The passphrase is hex, so it is the text between the + // quotes. + quoted := fields.AgePrivKeyPassphrase + if !bytes.HasPrefix(quoted, []byte(`"`)) { + return nil, errNoKeychainPassphrase + } + + return &KeychainData{ + AgePublicKey: fields.AgePublicKey, + // NewBufferFromBytes wipes the bytes it copies. + AgePrivKeyPassphrase: memguard.NewBufferFromBytes( + quoted[1 : len(quoted)-1]), + EncryptedLongtermKey: fields.EncryptedLongtermKey, + }, nil +} diff --git a/internal/secret/keychaindata_test.go b/internal/secret/keychaindata_test.go new file mode 100644 index 0000000..aa91f0e --- /dev/null +++ b/internal/secret/keychaindata_test.go @@ -0,0 +1,118 @@ +//nolint:testpackage // white-box test of unexported internals +package secret + +import ( + "encoding/json" + "testing" + + "github.com/awnumar/memguard" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGenerateRandomPassphrase(t *testing.T) { + t.Parallel() + + first, err := generateRandomPassphrase(64) + require.NoError(t, err) + + defer first.Destroy() + + second, err := generateRandomPassphrase(64) + require.NoError(t, err) + + defer second.Destroy() + + assert.Regexp(t, `^[0-9a-f]{64}$`, first.String()) + assert.NotEqual(t, first.String(), second.String()) + assert.False(t, first.IsMutable()) + + for _, length := range []int{0, -2, 63} { + _, err := generateRandomPassphrase(length) + require.ErrorIs(t, err, errPassphraseLength, "length %d", length) + } +} + +func TestKeychainDataEncodeDecode(t *testing.T) { + t.Parallel() + + passphrase := memguard.NewBufferFromBytes([]byte("0a1b2c3d")) + defer passphrase.Destroy() + + data := KeychainData{ + AgePublicKey: "age1example", + AgePrivKeyPassphrase: passphrase, + EncryptedLongtermKey: "beef", + } + + encoded, err := data.encode() + require.NoError(t, err) + + defer encoded.Destroy() + + assert.JSONEq(t, + `{"agePublicKey":"age1example",`+ + `"agePrivKeyPassphrase":"0a1b2c3d",`+ + `"encryptedLongtermKey":"beef"}`, + encoded.String()) + assert.False(t, encoded.IsMutable()) + + decoded, err := decodeKeychainData(encoded) + require.NoError(t, err) + + defer decoded.AgePrivKeyPassphrase.Destroy() + + assert.Equal(t, "age1example", decoded.AgePublicKey) + assert.Equal(t, "0a1b2c3d", decoded.AgePrivKeyPassphrase.String()) + assert.Equal(t, "beef", decoded.EncryptedLongtermKey) +} + +func TestKeychainDataEncodeRejectsBadPassphrase(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + passphrase *memguard.LockedBuffer + wantErr error + }{ + {"nil", nil, errNilPassphraseBuffer}, + {"empty", memguard.NewBuffer(0), errEmptyPassphrase}, + { + "not hex", + memguard.NewBufferFromBytes([]byte(`abc"def`)), + errPassphraseNotHex, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + data := KeychainData{AgePrivKeyPassphrase: tt.passphrase} + _, err := data.encode() + require.ErrorIs(t, err, tt.wantErr) + }) + } +} + +func TestDecodeKeychainDataRejectsBadData(t *testing.T) { + t.Parallel() + + for _, text := range []string{ + `{"agePublicKey":"age1example"}`, + `{"agePrivKeyPassphrase":42}`, + } { + data := memguard.NewBufferFromBytes([]byte(text)) + _, err := decodeKeychainData(data) + data.Destroy() + require.ErrorIs(t, err, errNoKeychainPassphrase, text) + } + + notJSON := memguard.NewBufferFromBytes([]byte(`{"agePrivKeyPassphrase":`)) + defer notJSON.Destroy() + + _, err := decodeKeychainData(notJSON) + + var syntaxError *json.SyntaxError + require.ErrorAs(t, err, &syntaxError) +} diff --git a/internal/secret/keychainunlocker.go b/internal/secret/keychainunlocker.go index c544214..0dfa54a 100644 --- a/internal/secret/keychainunlocker.go +++ b/internal/secret/keychainunlocker.go @@ -45,13 +45,6 @@ type KeychainUnlocker struct { fs afero.Fs } -// KeychainData represents the data stored in the macOS keychain -type KeychainData struct { - AgePublicKey string `json:"agePublicKey"` - AgePrivKeyPassphrase string `json:"agePrivKeyPassphrase"` - EncryptedLongtermKey string `json:"encryptedLongtermKey"` -} - // GetIdentity implements Unlocker interface for Keychain-based unlockers func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) { DebugWith("Getting keychain unlocker identity", @@ -81,13 +74,18 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) { slog.Int("data_length", len(keychainDataBytes)), ) + // Move the keychain data into locked memory; this wipes keychainDataBytes + keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes) + defer keychainDataBuffer.Destroy() + // Step 3: Parse keychain data - var keychainData KeychainData - if err := json.Unmarshal(keychainDataBytes, &keychainData); err != nil { + keychainData, err := decodeKeychainData(keychainDataBuffer) + if err != nil { Debug("Failed to parse keychain data", "error", err, "unlocker_id", k.GetID()) return nil, fmt.Errorf("failed to parse keychain data: %w", err) } + defer keychainData.AgePrivKeyPassphrase.Destroy() Debug("Parsed keychain data successfully", "unlocker_id", k.GetID()) @@ -109,11 +107,7 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) { // Step 5: Decrypt the age private key using the passphrase from keychain Debug("Decrypting age private key with keychain passphrase", "unlocker_id", k.GetID()) - // Create secure buffer for the keychain passphrase - passphraseBuffer := memguard.NewBufferFromBytes([]byte(keychainData.AgePrivKeyPassphrase)) - defer passphraseBuffer.Destroy() - - agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, passphraseBuffer) + agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, keychainData.AgePrivKeyPassphrase) if err != nil { Debug("Failed to decrypt age private key with keychain passphrase", "error", err, "unlocker_id", k.GetID()) @@ -369,6 +363,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er if err != nil { return nil, fmt.Errorf("failed to generate age private key passphrase: %w", err) } + defer agePrivKeyPassphrase.Destroy() // Step 3: Store age recipient as plaintext ageRecipient := ageIdentity.Recipient().String() @@ -378,15 +373,12 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er } // Step 4: Encrypt age private key with the generated passphrase and store on disk - // Create secure buffers for both the private key and passphrase + // Create a secure buffer for the private key agePrivKeyStr := ageIdentity.String() agePrivKeyBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyStr)) defer agePrivKeyBuffer.Destroy() - passphraseBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyPassphrase)) - defer passphraseBuffer.Destroy() - - encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, passphraseBuffer) + encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, agePrivKeyPassphrase) if err != nil { return nil, fmt.Errorf("failed to encrypt age private key with passphrase: %w", err) } @@ -422,13 +414,10 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er EncryptedLongtermKey: hex.EncodeToString(encryptedLtPrivKeyToAge), } - keychainDataBytes, err := json.Marshal(keychainData) + keychainDataBuffer, err := keychainData.encode() if err != nil { - return nil, fmt.Errorf("failed to marshal keychain data: %w", err) + return nil, fmt.Errorf("failed to encode keychain data: %w", err) } - - // Create a secure buffer for keychain data - keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes) defer keychainDataBuffer.Destroy() // Step 8: Store data in keychain @@ -501,7 +490,7 @@ func storeInKeychain(itemName string, data *memguard.LockedBuffer) error { item.SetAccount(itemName) item.SetLabel(fmt.Sprintf("%s - %s", KEYCHAIN_APP_IDENTIFIER, itemName)) item.SetDescription("Secret vault keychain data") - item.SetData([]byte(data.String())) + item.SetData(data.Bytes()) item.SetSynchronizable(keychain.SynchronizableNo) // Use AccessibleWhenUnlockedThisDeviceOnly for better security and to trigger auth item.SetAccessible(keychain.AccessibleWhenUnlockedThisDeviceOnly) @@ -576,8 +565,3 @@ func deleteFromKeychain(itemName string) error { return nil } - -// generateRandomPassphrase generates a random passphrase for encrypting the age private key -func generateRandomPassphrase(length int) (string, error) { - return generateRandomString(length, "0123456789abcdef") -} -- 2.54.0