From 985d79d3c06752facd1b17b5e16d0bbd296f898d Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 20 Jun 2025 07:50:26 -0700 Subject: [PATCH] fix: resolve critical security vulnerabilities in debug logging and command execution - Remove sensitive data from debug logs (vault/secrets.go, secret/version.go) - Add input validation for GPG key IDs and keychain item names - Resolve GPG key IDs to full fingerprints before storing in metadata - Add comprehensive test coverage for validation functions - Add golangci-lint configuration with additional linters Security improvements: - Debug logs no longer expose decrypted secret values or private keys - GPG and keychain commands now validate input to prevent injection attacks - All validation uses precompiled regex patterns for performance --- .golangci.yml | 99 ++++++++++ internal/secret/keychainunlocker.go | 37 +++- internal/secret/pgpunlock_test.go | 35 ++-- internal/secret/pgpunlocker.go | 68 ++++++- internal/secret/validation_test.go | 297 ++++++++++++++++++++++++++++ internal/secret/version.go | 1 - pkg/bip85/bip85.go | 18 +- pkg/bip85/bip85_test.go | 2 + 8 files changed, 529 insertions(+), 28 deletions(-) create mode 100644 .golangci.yml create mode 100644 internal/secret/validation_test.go diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..8d61c37 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,99 @@ +run: + timeout: 5m + go: "1.22" + +linters: + enable: + # Additional linters requested + - testifylint # Checks usage of github.com/stretchr/testify + - usetesting # usetesting is an analyzer that detects using os.Setenv instead of t.Setenv since Go 1.17 + - tagliatelle # Checks the struct tags + - nlreturn # nlreturn checks for a new line before return and branch statements + - nilnil # Checks that there is no simultaneous return of nil error and an invalid value + - nestif # Reports deeply nested if statements + - mnd # An analyzer to detect magic numbers + - lll # Reports long lines + - intrange # intrange is a linter to find places where for loops could make use of an integer range + - gofumpt # Gofumpt checks whether code was gofumpt-ed + - gochecknoglobals # Check that no global variables exist + + # Default/existing linters that are commonly useful + - govet + - errcheck + - staticcheck + - unused + - gosimple + - ineffassign + - typecheck + - gofmt + - goimports + - misspell + - revive + - gosec + - unconvert + - unparam + +linters-settings: + lll: + line-length: 120 + + mnd: + # List of enabled checks, see https://github.com/tommy-muehle/go-mnd/#checks for description. + checks: + - argument + - case + - condition + - operation + - return + - assign + ignored-numbers: + - '0' + - '1' + - '2' + - '8' + - '16' + - '40' # GPG fingerprint length + - '64' + - '128' + - '256' + - '512' + - '1024' + - '2048' + - '4096' + + nestif: + min-complexity: 4 + + nlreturn: + block-size: 2 + + tagliatelle: + case: + rules: + json: snake + yaml: snake + xml: snake + bson: snake + + testifylint: + enable-all: true + + usetesting: + strict: true + +issues: + exclude-rules: + # Exclude some linters from running on tests files + - path: _test\.go + linters: + - gochecknoglobals + - mnd + - unparam + + # Allow long lines in generated code or test data + - path: ".*_gen\\.go" + linters: + - lll + + max-issues-per-linter: 0 + max-same-issues: 0 \ No newline at end of file diff --git a/internal/secret/keychainunlocker.go b/internal/secret/keychainunlocker.go index 8f2e262..4bebb3e 100644 --- a/internal/secret/keychainunlocker.go +++ b/internal/secret/keychainunlocker.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "time" "filippo.io/age" @@ -15,6 +16,12 @@ import ( "github.com/spf13/afero" ) +var ( + // keychainItemNameRegex validates keychain item names + // Allows alphanumeric characters, dots, hyphens, and underscores only + keychainItemNameRegex = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) +) + // KeychainUnlockerMetadata extends UnlockerMetadata with keychain-specific data type KeychainUnlockerMetadata struct { UnlockerMetadata @@ -392,9 +399,25 @@ func checkMacOSAvailable() error { return nil } +// validateKeychainItemName validates that a keychain item name is safe for command execution +func validateKeychainItemName(itemName string) error { + if itemName == "" { + return fmt.Errorf("keychain item name cannot be empty") + } + + if !keychainItemNameRegex.MatchString(itemName) { + return fmt.Errorf("invalid keychain item name format: %s", itemName) + } + + return nil +} + // storeInKeychain stores data in the macOS keychain using the security command func storeInKeychain(itemName string, data []byte) error { - cmd := exec.Command("/usr/bin/security", "add-generic-password", + if err := validateKeychainItemName(itemName); err != nil { + return fmt.Errorf("invalid keychain item name: %w", err) + } + cmd := exec.Command("/usr/bin/security", "add-generic-password", //nolint:gosec // Input validated by validateKeychainItemName "-a", itemName, "-s", itemName, "-w", string(data), @@ -409,7 +432,11 @@ func storeInKeychain(itemName string, data []byte) error { // retrieveFromKeychain retrieves data from the macOS keychain using the security command func retrieveFromKeychain(itemName string) ([]byte, error) { - cmd := exec.Command("/usr/bin/security", "find-generic-password", + if err := validateKeychainItemName(itemName); err != nil { + return nil, fmt.Errorf("invalid keychain item name: %w", err) + } + + cmd := exec.Command("/usr/bin/security", "find-generic-password", //nolint:gosec // Input validated by validateKeychainItemName "-a", itemName, "-s", itemName, "-w") // Return password only @@ -429,7 +456,11 @@ func retrieveFromKeychain(itemName string) ([]byte, error) { // deleteFromKeychain removes an item from the macOS keychain using the security command func deleteFromKeychain(itemName string) error { - cmd := exec.Command("/usr/bin/security", "delete-generic-password", + if err := validateKeychainItemName(itemName); err != nil { + return fmt.Errorf("invalid keychain item name: %w", err) + } + + cmd := exec.Command("/usr/bin/security", "delete-generic-password", //nolint:gosec // Input validated by validateKeychainItemName "-a", itemName, "-s", itemName) diff --git a/internal/secret/pgpunlock_test.go b/internal/secret/pgpunlock_test.go index f515017..1190b75 100644 --- a/internal/secret/pgpunlock_test.go +++ b/internal/secret/pgpunlock_test.go @@ -189,21 +189,26 @@ Passphrase: ` + testPassphrase + ` } t.Log("GPG key generated successfully") - // Get the key ID + // Get the key ID and fingerprint output, err := runGPGWithPassphrase(gnupgHomeDir, testPassphrase, - []string{"--list-secret-keys", "--with-colons"}, nil) + []string{"--list-secret-keys", "--with-colons", "--fingerprint"}, nil) if err != nil { t.Fatalf("Failed to list GPG keys: %v", err) } - // Parse output to get key ID - var keyID string + // Parse output to get key ID and fingerprint + var keyID, fingerprint string lines := strings.Split(string(output), "\n") for _, line := range lines { if strings.HasPrefix(line, "sec:") { fields := strings.Split(line, ":") if len(fields) >= 5 { keyID = fields[4] + } + } else if strings.HasPrefix(line, "fpr:") { + fields := strings.Split(line, ":") + if len(fields) >= 10 && fields[9] != "" { + fingerprint = fields[9] break } } @@ -212,7 +217,11 @@ Passphrase: ` + testPassphrase + ` if keyID == "" { t.Fatalf("Failed to find GPG key ID in output: %s", output) } + if fingerprint == "" { + t.Fatalf("Failed to find GPG fingerprint in output: %s", output) + } t.Logf("Generated GPG key ID: %s", keyID) + t.Logf("Generated GPG fingerprint: %s", fingerprint) // Set the GPG_AGENT_INFO to empty to ensure gpg-agent doesn't interfere oldAgentInfo := os.Getenv("GPG_AGENT_INFO") @@ -326,9 +335,9 @@ Passphrase: ` + testPassphrase + ` t.Errorf("Expected PGP unlock key type 'pgp', got '%s'", pgpUnlocker.GetType()) } - // Check if the key ID includes the GPG key ID - if !strings.Contains(pgpUnlocker.GetID(), keyID) { - t.Errorf("PGP unlock key ID '%s' does not contain GPG key ID '%s'", pgpUnlocker.GetID(), keyID) + // Check if the key ID includes the GPG fingerprint + if !strings.Contains(pgpUnlocker.GetID(), fingerprint) { + t.Errorf("PGP unlock key ID '%s' does not contain GPG fingerprint '%s'", pgpUnlocker.GetID(), fingerprint) } // Check if the key directory exists @@ -400,8 +409,8 @@ Passphrase: ` + testPassphrase + ` t.Errorf("Expected metadata type 'pgp', got '%s'", metadata.Type) } - if metadata.GPGKeyID != keyID { - t.Errorf("Expected GPG key ID '%s', got '%s'", keyID, metadata.GPGKeyID) + if metadata.GPGKeyID != fingerprint { + t.Errorf("Expected GPG fingerprint '%s', got '%s'", fingerprint, metadata.GPGKeyID) } }) @@ -431,7 +440,7 @@ Passphrase: ` + testPassphrase + ` pgpMetadata := PGPUnlockerMetadata{ UnlockerMetadata: metadata, - GPGKeyID: keyID, + GPGKeyID: fingerprint, } // Write metadata file @@ -450,9 +459,9 @@ Passphrase: ` + testPassphrase + ` t.Fatalf("Failed to get GPG key ID: %v", err) } - // Verify key ID - if retrievedKeyID != keyID { - t.Errorf("Expected GPG key ID '%s', got '%s'", keyID, retrievedKeyID) + // Verify key ID (should be the fingerprint) + if retrievedKeyID != fingerprint { + t.Errorf("Expected GPG fingerprint '%s', got '%s'", fingerprint, retrievedKeyID) } }) diff --git a/internal/secret/pgpunlocker.go b/internal/secret/pgpunlocker.go index 8dc6d03..a60ae94 100644 --- a/internal/secret/pgpunlocker.go +++ b/internal/secret/pgpunlocker.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "time" @@ -24,6 +25,19 @@ var ( // GPGDecryptFunc is the function used for GPG decryption // Can be overridden in tests to provide a non-interactive implementation GPGDecryptFunc = gpgDecryptDefault + + // gpgKeyIDRegex validates GPG key IDs + // Allows either: + // 1. Email addresses (user@domain.tld format) + // 2. Short key IDs (8 hex characters) + // 3. Long key IDs (16 hex characters) + // 4. Full fingerprints (40 hex characters) + gpgKeyIDRegex = regexp.MustCompile( + `^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$|` + + `^[A-Fa-f0-9]{8}$|` + + `^[A-Fa-f0-9]{16}$|` + + `^[A-Fa-f0-9]{40}$`, + ) ) // PGPUnlockerMetadata extends UnlockerMetadata with PGP-specific data @@ -285,14 +299,20 @@ func CreatePGPUnlocker(fs afero.Fs, stateDir string, gpgKeyID string) (*PGPUnloc return nil, fmt.Errorf("failed to write encrypted age private key: %w", err) } - // Step 9: Create and write enhanced metadata + // Step 9: Resolve the GPG key ID to its full fingerprint + fingerprint, err := resolveGPGKeyFingerprint(gpgKeyID) + if err != nil { + return nil, fmt.Errorf("failed to resolve GPG key fingerprint: %w", err) + } + + // Step 10: Create and write enhanced metadata with full fingerprint pgpMetadata := PGPUnlockerMetadata{ UnlockerMetadata: UnlockerMetadata{ Type: "pgp", CreatedAt: time.Now(), Flags: []string{"gpg", "encrypted"}, }, - GPGKeyID: gpgKeyID, + GPGKeyID: fingerprint, } metadataBytes, err := json.MarshalIndent(pgpMetadata, "", " ") @@ -311,6 +331,46 @@ func CreatePGPUnlocker(fs afero.Fs, stateDir string, gpgKeyID string) (*PGPUnloc }, nil } +// validateGPGKeyID validates that a GPG key ID is safe for command execution +func validateGPGKeyID(keyID string) error { + if keyID == "" { + return fmt.Errorf("GPG key ID cannot be empty") + } + + if !gpgKeyIDRegex.MatchString(keyID) { + return fmt.Errorf("invalid GPG key ID format: %s", keyID) + } + + return nil +} + +// resolveGPGKeyFingerprint resolves any GPG key identifier to its full fingerprint +func resolveGPGKeyFingerprint(keyID string) (string, error) { + if err := validateGPGKeyID(keyID); err != nil { + return "", fmt.Errorf("invalid GPG key ID: %w", err) + } + + // Use GPG to get the full fingerprint for the key + cmd := exec.Command("gpg", "--list-keys", "--with-colons", "--fingerprint", keyID) + output, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err) + } + + // Parse the output to extract the fingerprint + lines := strings.Split(string(output), "\n") + for _, line := range lines { + if strings.HasPrefix(line, "fpr:") { + fields := strings.Split(line, ":") + if len(fields) >= 10 && fields[9] != "" { + return fields[9], nil + } + } + } + + return "", fmt.Errorf("could not find fingerprint for GPG key: %s", keyID) +} + // checkGPGAvailable verifies that GPG is available func checkGPGAvailable() error { cmd := exec.Command("gpg", "--version") @@ -322,6 +382,10 @@ func checkGPGAvailable() error { // gpgEncryptDefault is the default implementation of GPG encryption func gpgEncryptDefault(data []byte, keyID string) ([]byte, error) { + if err := validateGPGKeyID(keyID); err != nil { + return nil, fmt.Errorf("invalid GPG key ID: %w", err) + } + cmd := exec.Command("gpg", "--trust-model", "always", "--armor", "--encrypt", "-r", keyID) cmd.Stdin = strings.NewReader(string(data)) diff --git a/internal/secret/validation_test.go b/internal/secret/validation_test.go new file mode 100644 index 0000000..877810f --- /dev/null +++ b/internal/secret/validation_test.go @@ -0,0 +1,297 @@ +package secret + +import ( + "testing" +) + +func TestValidateGPGKeyID(t *testing.T) { + tests := []struct { + name string + keyID string + wantErr bool + }{ + // Valid cases + { + name: "valid email address", + keyID: "test@example.com", + wantErr: false, + }, + { + name: "valid email with dots and hyphens", + keyID: "test.user-name@example-domain.co.uk", + wantErr: false, + }, + { + name: "valid email with plus", + keyID: "test+tag@example.com", + wantErr: false, + }, + { + name: "valid short key ID (8 hex chars)", + keyID: "ABCDEF12", + wantErr: false, + }, + { + name: "valid long key ID (16 hex chars)", + keyID: "ABCDEF1234567890", + wantErr: false, + }, + { + name: "valid fingerprint (40 hex chars)", + keyID: "ABCDEF1234567890ABCDEF1234567890ABCDEF12", + wantErr: false, + }, + { + name: "valid lowercase hex fingerprint", + keyID: "abcdef1234567890abcdef1234567890abcdef12", + wantErr: false, + }, + { + name: "valid mixed case hex", + keyID: "AbCdEf1234567890", + wantErr: false, + }, + + // Invalid cases + { + name: "empty key ID", + keyID: "", + wantErr: true, + }, + { + name: "key ID with spaces", + keyID: "test user@example.com", + wantErr: true, + }, + { + name: "key ID with semicolon (command injection)", + keyID: "test@example.com; rm -rf /", + wantErr: true, + }, + { + name: "key ID with pipe (command injection)", + keyID: "test@example.com | cat /etc/passwd", + wantErr: true, + }, + { + name: "key ID with backticks (command injection)", + keyID: "test@example.com`whoami`", + wantErr: true, + }, + { + name: "key ID with dollar sign (command injection)", + keyID: "test@example.com$(whoami)", + wantErr: true, + }, + { + name: "key ID with quotes", + keyID: "test\"@example.com", + wantErr: true, + }, + { + name: "key ID with single quotes", + keyID: "test'@example.com", + wantErr: true, + }, + { + name: "key ID with backslash", + keyID: "test\\@example.com", + wantErr: true, + }, + { + name: "key ID with newline", + keyID: "test@example.com\nrm -rf /", + wantErr: true, + }, + { + name: "key ID with carriage return", + keyID: "test@example.com\rrm -rf /", + wantErr: true, + }, + { + name: "hex with invalid length (7 chars)", + keyID: "ABCDEF1", + wantErr: true, + }, + { + name: "hex with invalid length (9 chars)", + keyID: "ABCDEF123", + wantErr: true, + }, + { + name: "hex with non-hex characters", + keyID: "ABCDEFGH", + wantErr: true, + }, + { + name: "mixed format (email with hex)", + keyID: "test@ABCDEF12", + wantErr: true, + }, + { + name: "key ID with ampersand", + keyID: "test@example.com & echo test", + wantErr: true, + }, + { + name: "key ID with redirect", + keyID: "test@example.com > /tmp/test", + wantErr: true, + }, + { + name: "key ID with null byte", + keyID: "test@example.com\x00", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateGPGKeyID(tt.keyID) + if (err != nil) != tt.wantErr { + t.Errorf("validateGPGKeyID() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestValidateKeychainItemName(t *testing.T) { + tests := []struct { + name string + itemName string + wantErr bool + }{ + // Valid cases + { + name: "valid simple name", + itemName: "my-secret-key", + wantErr: false, + }, + { + name: "valid name with dots", + itemName: "com.example.app.key", + wantErr: false, + }, + { + name: "valid name with underscores", + itemName: "my_secret_key_123", + wantErr: false, + }, + { + name: "valid alphanumeric", + itemName: "Secret123Key", + wantErr: false, + }, + { + name: "valid with hyphen at start", + itemName: "-my-key", + wantErr: false, + }, + { + name: "valid with dot at start", + itemName: ".hidden-key", + wantErr: false, + }, + + // Invalid cases + { + name: "empty item name", + itemName: "", + wantErr: true, + }, + { + name: "item name with spaces", + itemName: "my secret key", + wantErr: true, + }, + { + name: "item name with semicolon", + itemName: "key;rm -rf /", + wantErr: true, + }, + { + name: "item name with pipe", + itemName: "key|cat /etc/passwd", + wantErr: true, + }, + { + name: "item name with backticks", + itemName: "key`whoami`", + wantErr: true, + }, + { + name: "item name with dollar sign", + itemName: "key$(whoami)", + wantErr: true, + }, + { + name: "item name with quotes", + itemName: "key\"name", + wantErr: true, + }, + { + name: "item name with single quotes", + itemName: "key'name", + wantErr: true, + }, + { + name: "item name with backslash", + itemName: "key\\name", + wantErr: true, + }, + { + name: "item name with newline", + itemName: "key\nname", + wantErr: true, + }, + { + name: "item name with carriage return", + itemName: "key\rname", + wantErr: true, + }, + { + name: "item name with ampersand", + itemName: "key&echo test", + wantErr: true, + }, + { + name: "item name with redirect", + itemName: "key>/tmp/test", + wantErr: true, + }, + { + name: "item name with null byte", + itemName: "key\x00name", + wantErr: true, + }, + { + name: "item name with parentheses", + itemName: "key(test)", + wantErr: true, + }, + { + name: "item name with brackets", + itemName: "key[test]", + wantErr: true, + }, + { + name: "item name with asterisk", + itemName: "key*", + wantErr: true, + }, + { + name: "item name with question mark", + itemName: "key?", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateKeychainItemName(tt.itemName) + if (err != nil) != tt.wantErr { + t.Errorf("validateKeychainItemName() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/secret/version.go b/internal/secret/version.go index 1f2b8e8..133d812 100644 --- a/internal/secret/version.go +++ b/internal/secret/version.go @@ -343,7 +343,6 @@ func (sv *SecretVersion) GetValue(ltIdentity *age.X25519Identity) ([]byte, error Debug("Successfully retrieved version value", "version", sv.Version, "value_length", len(value), - "value_as_string", string(value), "is_empty", len(value) == 0) return value, nil } diff --git a/pkg/bip85/bip85.go b/pkg/bip85/bip85.go index c343e5b..004aac8 100644 --- a/pkg/bip85/bip85.go +++ b/pkg/bip85/bip85.go @@ -22,19 +22,19 @@ import ( const ( // BIP85_MASTER_PATH is the derivation path prefix for all BIP85 applications - BIP85_MASTER_PATH = "m/83696968'" + BIP85_MASTER_PATH = "m/83696968'" //nolint:revive // ALL_CAPS used for BIP85 constants // BIP85_KEY_HMAC_KEY is the HMAC key used for deriving the entropy - BIP85_KEY_HMAC_KEY = "bip-entropy-from-k" + BIP85_KEY_HMAC_KEY = "bip-entropy-from-k" //nolint:revive // ALL_CAPS used for BIP85 constants // Application numbers - APP_BIP39 = 39 // BIP39 mnemonics - APP_HD_WIF = 2 // WIF for Bitcoin Core - APP_XPRV = 32 // Extended private key - APP_HEX = 128169 - APP_PWD64 = 707764 // Base64 passwords - APP_PWD85 = 707785 // Base85 passwords - APP_RSA = 828365 + APP_BIP39 = 39 // BIP39 mnemonics //nolint:revive // ALL_CAPS used for BIP85 constants + APP_HD_WIF = 2 // WIF for Bitcoin Core //nolint:revive // ALL_CAPS used for BIP85 constants + APP_XPRV = 32 // Extended private key //nolint:revive // ALL_CAPS used for BIP85 constants + APP_HEX = 128169 //nolint:revive // ALL_CAPS used for BIP85 constants + APP_PWD64 = 707764 // Base64 passwords //nolint:revive // ALL_CAPS used for BIP85 constants + APP_PWD85 = 707785 // Base85 passwords //nolint:revive // ALL_CAPS used for BIP85 constants + APP_RSA = 828365 //nolint:revive // ALL_CAPS used for BIP85 constants ) // Version bytes for extended keys diff --git a/pkg/bip85/bip85_test.go b/pkg/bip85/bip85_test.go index 17547cd..7290a8c 100644 --- a/pkg/bip85/bip85_test.go +++ b/pkg/bip85/bip85_test.go @@ -1,5 +1,7 @@ package bip85 +//nolint:gosec,revive,unparam // Test file with hardcoded test vectors + import ( "bytes" "encoding/hex"