diff --git a/SECURITY-FINDINGS.md b/SECURITY-FINDINGS.md new file mode 100644 index 0000000..78e3d65 --- /dev/null +++ b/SECURITY-FINDINGS.md @@ -0,0 +1,238 @@ +# Security Audit: Pop CLI + +**Issue:** [FRE-684](/FRE/issues/FRE-684) +**Auditor:** Security Reviewer +**Date:** 2026-04-26 +**Scope:** PGP key handling, token storage, API security, OWASP Top 10 for CLI + +--- + +## Executive Summary + +**14 findings:** 3 Critical, 5 High, 4 Medium, 2 Low + +The most severe issue is that PGP encrypt/decrypt operations are complete no-ops — all mail traffic is effectively unencrypted despite the tool claiming PGP support. + +--- + +## Critical Findings + +### C-1: PGP Encrypt/Decrypt Are No-Operations (CWE-347) + +**Files:** `internal/mail/pgp.go:38-48,79-81` +**Severity:** Critical + +The Encrypt, Decrypt, SignData, and EncryptAndSign methods all return their inputs unchanged without performing any cryptographic operation. + +**Impact:** All "encrypted" email is sent as plaintext. All "decrypted" email shows raw encrypted data to the user. PGP signing provides zero integrity. This completely defeats the purpose of a ProtonMail CLI tool. + +**Remediation:** Implement actual PGP operations using gopenpgp's crypto package methods for encryption, decryption, and signing. + +--- + +### C-2: Attachment Encryption Is a No-Op (CWE-347) + +**Files:** `internal/mail/pgp.go:83-111` +**Severity:** Critical + +EncryptAttachment generates a random symmetric key but copies the plaintext data without encrypting it. DecryptAttachment copies the "encrypted" data back without decrypting. + +**Impact:** Attachments are stored and transmitted in plaintext. The symmetric key is generated but never applied. + +**Remediation:** Use the symmetric key with AES-GCM to encrypt data, then encrypt the symmetric key with the recipient's public key using PGP. + +--- + +### C-3: Session Credentials Stored Unencrypted on Disk (CWE-312) + +**Files:** `internal/auth/session.go:44-55` +**Severity:** Critical + +Access tokens and refresh tokens are stored as plaintext JSON in `~/.config/pop/session.json`. + +**Impact:** Any process or user with read access can extract valid session credentials. On a shared system, this enables account takeover. + +**Remediation:** +1. Encrypt session file at rest using OS keyring (libsecret on Linux, Keychain on macOS) +2. Use `github.com/99designs/keyring` for cross-platform secure storage +3. Session file permissions are correct (0600) but directory is 0755 — reduce to 0700 + +--- + +## High Findings + +### H-1: Passphrase Transmitted in URL Query Parameters (CWE-319) + +**Files:** `internal/mail/client.go:30,75,107,195,245,317` +**Severity:** High + +The Passphrase is sent as a URL query parameter across 6+ endpoints via `params.Set("Passphrase", req.Passphrase)`. + +**Impact:** Passphrases appear in server access logs, proxy logs, process memory dumps, and any middleware that logs URLs. + +**Remediation:** Send passphrase in the request body, not query parameters. + +--- + +### H-2: Password Acceptable via Command-Line Flag (CWE-798) + +**Files:** `cmd/auth.go:25` +**Severity:** High + +`--password/-p` flag allows passwords on the command line. + +**Impact:** Passwords appear in `ps` output visible to all local users, shell history, and process monitors. + +**Remediation:** Remove `--password` flag entirely. Use only interactive prompt with `golang.org/x/term` for masked input. + +--- + +### H-3: Config Directory Permissions Too Permissive (CWE-732) + +**Files:** `internal/config/config.go:64`, `internal/auth/session.go:44`, `internal/attachment/manager.go:20,40` +**Severity:** High + +`os.MkdirAll(m.configDir, 0755)` creates a world-readable config directory. + +**Impact:** Other users can list files in `~/.config/pop/` and potentially read contents. + +**Remediation:** Change all `os.MkdirAll` calls for config directories to use `0700`. + +--- + +### H-4: No Token Expiration Validation (CWE-613) + +**Files:** `internal/auth/session.go:84-89` +**Severity:** High + +`IsAuthenticated()` checks file existence but never validates `ExpiresAt`. No token refresh logic exists. + +**Impact:** Expired tokens are used for API requests, causing silent authentication failures. + +**Remediation:** +1. Check `time.Now().Unix() > session.ExpiresAt` before returning authenticated +2. Implement token refresh using RefreshToken before expiry +3. Add retry logic with automatic refresh on 401 responses + +--- + +### H-5: RSA 2048-Key Generation Below Current Standards (CWE-326) + +**Files:** `internal/mail/pgp.go:51` +**Severity:** High + +`crypto.GenerateKey(email, passphrase, "RSA", 2048)` uses RSA-2048. + +**Impact:** RSA-2048 provides approximately 112 bits of security. NIST recommends minimum RSA-3072 for protection through 2030. + +**Remediation:** Increase to RSA-4096 or use ECC (Curve25519). + +--- + +## Medium Findings + +### M-1: No TLS Certificate Pinning (CWE-295) + +**Files:** `internal/api/client.go:31-42` +**Severity:** Medium + +Standard http.Client with no custom Transport or cert pinning. + +**Impact:** A compromised CA or MITM attack with a valid certificate could intercept all API traffic. + +**Remediation:** Consider adding certificate pinning for the ProtonMail API endpoint. + +--- + +### M-2: API Error Messages May Leak Internal Details (CWE-209) + +**Files:** `internal/api/client.go:102-108`, `internal/mail/client.go:141-142,164-165,185-186` +**Severity:** Medium + +Raw API error bodies are included in user-facing error messages. + +**Impact:** API error responses may contain internal server details, stack traces, or sensitive metadata. + +**Remediation:** Sanitize error responses before displaying. Log full details internally, show generic messages to users. + +--- + +### M-3: No Input Validation on Email Addresses (CWE-20) + +**Files:** `cmd/mail.go:379-395` +**Severity:** Medium + +`parseRecipients` performs no format validation on email addresses. + +**Impact:** Malformed addresses, header injection via newlines, or crafted addresses could cause issues. + +**Remediation:** Validate email addresses using format checks before sending. + +--- + +### M-4: Sensitive Data Not Zeroed from Memory (CWE-226) + +**Severity:** Medium + +Passphrases, tokens, and private keys are stored in Go strings which cannot be zeroed. + +**Impact:** Memory dumps, swap files, or core dumps could contain sensitive cryptographic material. + +**Remediation:** Use `[]byte` slices for sensitive data and explicitly zero them after use. + +--- + +## Low Findings + +### L-1: Attachment Files Stored With World-Readable Permissions (CWE-732) + +**Files:** `internal/attachment/manager.go:36,51` +**Severity:** Low + +`os.WriteFile(dest, data, 0644)` creates world-readable attachment files. + +**Remediation:** Use `0600` for attachment files. + +--- + +### L-2: No Confirmation for Destructive Operations (CWE-710) + +**Files:** `cmd/mail.go:247-282` +**Severity:** Low + +`mail delete` permanently deletes messages without confirmation. + +**Remediation:** Add `--yes`/`--force` flag requiring explicit confirmation. + +--- + +## Summary Table + +| ID | Severity | Title | CWE | File | +|----|----------|-------|-----|------| +| C-1 | Critical | PGP Encrypt/Decrypt no-ops | CWE-347 | pgp.go:38-48 | +| C-2 | Critical | Attachment encryption no-op | CWE-347 | pgp.go:83-111 | +| C-3 | Critical | Session tokens unencrypted at rest | CWE-312 | session.go:44-55 | +| H-1 | High | Passphrase in URL params | CWE-319 | client.go:30,75 | +| H-2 | High | Password via CLI flag | CWE-798 | auth.go:25 | +| H-3 | High | Config dir 0755 permissions | CWE-732 | config.go:64 | +| H-4 | High | No token expiry check | CWE-613 | session.go:84-89 | +| H-5 | High | RSA-2048 below standards | CWE-326 | pgp.go:51 | +| M-1 | Medium | No TLS cert pinning | CWE-295 | client.go:31 | +| M-2 | Medium | API error info leak | CWE-209 | client.go:102 | +| M-3 | Medium | No email validation | CWE-20 | mail.go:379 | +| M-4 | Medium | Sensitive data in memory | CWE-226 | throughout | +| L-1 | Low | Attachment files 0644 | CWE-732 | manager.go:36 | +| L-2 | Low | No delete confirmation | CWE-710 | mail.go:247 | + +--- + +## Priority Remediation Order + +1. **C-1, C-2** — Implement actual PGP encryption (blocks all secure usage) +2. **C-3** — Encrypt session storage or use OS keyring +3. **H-1** — Move passphrase from URL params to request body +4. **H-2** — Remove `--password` CLI flag +5. **H-3** — Fix directory permissions to 0700 +6. **H-4** — Add token expiry validation and refresh +7. **H-5** — Increase RSA key size to 4096