Fix remaining memory security issues
- Fixed gpgDecryptDefault to return *memguard.LockedBuffer instead of []byte - Updated GPGDecryptFunc signature and all implementations - Confirmed getSecretValue already returns LockedBuffer (was fixed earlier) - Improved passphrase string handling by removing intermediate variables - Note: String conversion for passphrases is unavoidable due to age library API - All GPG decrypted data is now immediately protected in memory
This commit is contained in:
parent
ff17b9b107
commit
e53161188c
6
TODO.md
6
TODO.md
@ -16,11 +16,11 @@ prioritized from most critical (top) to least critical (bottom).
|
||||
- [x] **5. GetValue returns unprotected secret**: `internal/secret/secret.go:93` - `GetValue(unlocker Unlocker) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer internally
|
||||
- [x] **6. DecryptWithIdentity returns unprotected data**: `internal/secret/crypto.go:57` - `DecryptWithIdentity(data []byte, identity age.Identity) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
|
||||
- [x] **7. DecryptWithPassphrase returns unprotected data**: `internal/secret/crypto.go:94` - `DecryptWithPassphrase(encryptedData []byte, passphrase *memguard.LockedBuffer) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
|
||||
- [ ] **8. gpgDecryptDefault returns unprotected data**: `internal/secret/pgpunlocker.go:368` - `gpgDecryptDefault(encryptedData []byte) ([]byte, error)` - returns decrypted data unprotected
|
||||
- [ ] **9. getSecretValue returns unprotected data**: `internal/cli/crypto.go:269` - `getSecretValue()` returns bare []byte
|
||||
- [x] **8. gpgDecryptDefault returns unprotected data**: `internal/secret/pgpunlocker.go:368` - `gpgDecryptDefault(encryptedData []byte) ([]byte, error)` - ✓ FIXED - now returns LockedBuffer
|
||||
- [x] **9. getSecretValue returns unprotected data**: `internal/cli/crypto.go:269` - `getSecretValue()` returns bare []byte - ✓ ALREADY FIXED - returns LockedBuffer
|
||||
|
||||
### Intermediate string variables for passphrases
|
||||
- [ ] **10. Passphrase extracted to string**: `internal/secret/crypto.go:79,100` - `passphraseStr := passphrase.String()` - passphrase extracted to plain string before use with age.NewScryptRecipient
|
||||
- [x] **10. Passphrase extracted to string**: `internal/secret/crypto.go:79,100` - `passphraseStr := passphrase.String()` - ✓ UNAVOIDABLE - age library requires string parameter
|
||||
- [ ] **11. Age secret key in plain string**: `internal/cli/crypto.go:86,91,113` - Age secret key stored in plain string variable before conversion back to secure buffer
|
||||
|
||||
### Unprotected buffer.Bytes() usage
|
||||
|
@ -81,9 +81,8 @@ func EncryptWithPassphrase(data *memguard.LockedBuffer, passphrase *memguard.Loc
|
||||
return nil, fmt.Errorf("passphrase buffer is nil")
|
||||
}
|
||||
|
||||
// Get the passphrase string temporarily
|
||||
passphraseStr := passphrase.String()
|
||||
recipient, err := age.NewScryptRecipient(passphraseStr)
|
||||
// Create recipient directly from passphrase - unavoidable string conversion due to age API
|
||||
recipient, err := age.NewScryptRecipient(passphrase.String())
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to create scrypt recipient: %w", err)
|
||||
}
|
||||
@ -98,9 +97,8 @@ func DecryptWithPassphrase(encryptedData []byte, passphrase *memguard.LockedBuff
|
||||
return nil, fmt.Errorf("passphrase buffer is nil")
|
||||
}
|
||||
|
||||
// Get the passphrase string temporarily
|
||||
passphraseStr := passphrase.String()
|
||||
identity, err := age.NewScryptIdentity(passphraseStr)
|
||||
// Create identity directly from passphrase - unavoidable string conversion due to age API
|
||||
identity, err := age.NewScryptIdentity(passphrase.String())
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to create scrypt identity: %w", err)
|
||||
}
|
||||
|
@ -72,7 +72,7 @@ pinentry-mode loopback
|
||||
return stdout.Bytes(), nil
|
||||
}
|
||||
|
||||
secret.GPGDecryptFunc = func(encryptedData []byte) ([]byte, error) {
|
||||
secret.GPGDecryptFunc = func(encryptedData []byte) (*memguard.LockedBuffer, error) {
|
||||
cmd := exec.Command("gpg",
|
||||
"--homedir", gnupgHomeDir,
|
||||
"--batch",
|
||||
@ -91,7 +91,8 @@ pinentry-mode loopback
|
||||
return nil, fmt.Errorf("GPG decryption failed: %w\nStderr: %s", err, stderr.String())
|
||||
}
|
||||
|
||||
return stdout.Bytes(), nil
|
||||
// Create a secure buffer for the decrypted data
|
||||
return memguard.NewBufferFromBytes(stdout.Bytes()), nil
|
||||
}
|
||||
|
||||
// Restore original functions after test
|
||||
|
@ -25,7 +25,8 @@ var (
|
||||
|
||||
// GPGDecryptFunc is the function used for GPG decryption
|
||||
// Can be overridden in tests to provide a non-interactive implementation
|
||||
GPGDecryptFunc = gpgDecryptDefault //nolint:gochecknoglobals // Required for test mocking
|
||||
//nolint:gochecknoglobals // Required for test mocking
|
||||
GPGDecryptFunc func(encryptedData []byte) (*memguard.LockedBuffer, error) = gpgDecryptDefault
|
||||
|
||||
// gpgKeyIDRegex validates GPG key IDs
|
||||
// Allows either:
|
||||
@ -80,21 +81,22 @@ func (p *PGPUnlocker) GetIdentity() (*age.X25519Identity, error) {
|
||||
|
||||
// Step 2: Decrypt the age private key using GPG
|
||||
Debug("Decrypting age private key with GPG", "unlocker_id", p.GetID())
|
||||
agePrivKeyData, err := GPGDecryptFunc(encryptedAgePrivKeyData)
|
||||
agePrivKeyBuffer, err := GPGDecryptFunc(encryptedAgePrivKeyData)
|
||||
if err != nil {
|
||||
Debug("Failed to decrypt age private key with GPG", "error", err, "unlocker_id", p.GetID())
|
||||
|
||||
return nil, fmt.Errorf("failed to decrypt age private key with GPG: %w", err)
|
||||
}
|
||||
defer agePrivKeyBuffer.Destroy()
|
||||
|
||||
DebugWith("Successfully decrypted age private key with GPG",
|
||||
slog.String("unlocker_id", p.GetID()),
|
||||
slog.Int("decrypted_length", len(agePrivKeyData)),
|
||||
slog.Int("decrypted_length", agePrivKeyBuffer.Size()),
|
||||
)
|
||||
|
||||
// Step 3: Parse the decrypted age private key
|
||||
Debug("Parsing decrypted age private key", "unlocker_id", p.GetID())
|
||||
ageIdentity, err := age.ParseX25519Identity(string(agePrivKeyData))
|
||||
ageIdentity, err := age.ParseX25519Identity(agePrivKeyBuffer.String())
|
||||
if err != nil {
|
||||
Debug("Failed to parse age private key", "error", err, "unlocker_id", p.GetID())
|
||||
|
||||
@ -369,7 +371,7 @@ func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error
|
||||
}
|
||||
|
||||
// gpgDecryptDefault is the default implementation of GPG decryption
|
||||
func gpgDecryptDefault(encryptedData []byte) ([]byte, error) {
|
||||
func gpgDecryptDefault(encryptedData []byte) (*memguard.LockedBuffer, error) {
|
||||
cmd := exec.Command("gpg", "--quiet", "--decrypt")
|
||||
cmd.Stdin = strings.NewReader(string(encryptedData))
|
||||
|
||||
@ -378,5 +380,8 @@ func gpgDecryptDefault(encryptedData []byte) ([]byte, error) {
|
||||
return nil, fmt.Errorf("GPG decryption failed: %w", err)
|
||||
}
|
||||
|
||||
return output, nil
|
||||
// Create a secure buffer for the decrypted data
|
||||
outputBuffer := memguard.NewBufferFromBytes(output)
|
||||
|
||||
return outputBuffer, nil
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user