Automated Security Audit Results

[0xRahim]

Architecture audit — phrase_cli

What's actually solid

The overall layered design is well-thought-out. Every entry gets its own unique AES-256 session key (avoiding catastrophic nonce reuse), the GCM tag is always verified before returning plaintext, the clipboard is cleared on Enter, and rpassword is used for all sensitive input so the password never echoes. HKDF-SHA256 (not raw SHA-256) is used for the ECIES key derivation in session_key.rs, which is correct. The blob wire formats are self-contained (nonce prepended, no separate nonce storage). These are all good, deliberate choices.

---

Issue 1 — Critical: S2K is SHA-256, not Argon2id

This is the most serious finding. When you create a vault, the private key stored in enc_private_key is protected by the pgp crate's S2K mechanism, which uses iterated+salted SHA-256 → AES-256-CFB. SHA-256 is computationally cheap — a modern GPU can compute billions of SHA-256 hashes per second. An attacker who steals your .db file can run an offline dictionary attack against the encrypted private key at full GPU speed.

Meanwhile, session_key.rs already has a correct Argon2id KDF (m=64MB, t=3) — it's used to wrap the standalone X25519 private key in encrypt_x25519_private_key_with_mpass(). But that function is never called in the actual vault creation path. The vault uses PGP S2K instead.

Fix: Stop storing the PGP-protected ASC in the vault. Instead, use generate_x25519_keypair() + encrypt_x25519_private_key_with_mpass() from session_key.rs directly. Store the Argon2id-protected 76-byte blob as enc_private_key and the raw 32-byte public key (or its base64) as public_key. This removes the PGP dependency entirely from the storage path and gets you Argon2id at rest.

---

Issue 2 — The key extraction path is fragile

private_key_asc_to_x25519_bytes() derives an X25519 scalar from an Ed25519 seed by computing SHA-512(ed25519_seed)[0..32] and clamping. This is the libsodium "sign + encrypt from one key" pattern, and it works — but it tightly couples encryption to the PGP key type. If you ever rotate to a different PGP key type (e.g. Curve25519/v5 keys, which use a native X25519 subkey), this derivation would produce the wrong result and silently corrupt all existing vaults. The safer architecture is the standalone X25519 keypair already in session_key.rs.

---

Issue 3 — Yes, co-location is acceptable here, but the framing matters

Both public_key and enc_private_key live in the same vaults table. You asked whether this is right. The honest answer: co-location is fine if and only if the private key protection is strong. This is exactly what 1Password, Bitwarden, and every other serious password manager does — the encrypted private key lives next to the public key. The public key is not secret. The attacker's only prize from stealing the database is the encrypted private key, and their ability to do anything with it is bounded entirely by the KDF strength.

With Argon2id (issue 1 fixed), co-location is completely fine. With SHA-256 S2K, co-location means the attacker can mount a fast offline attack with both halves of the key relationship immediately available.

One legitimate improvement regardless: encrypt the alias, category, and entry_type fields in the entries table. Right now those are stored in plaintext, so an attacker who steals the database can enumerate all your account names and entry types even without cracking the master password. That's a metadata leak.

---

Issue 4 — Master password not zeroed after use

In commands.rs, mpass is a plain String. After deriving the X25519 key it stays allocated on the heap until the garbage collector (Rust's allocator) eventually overwrites it, which could be never. The zeroize crate is already a dependency (used in keys.rs). Wrap the password in Zeroizing<String> at the point it's read from rpassword and it will be wiped on drop.

The x25519_priv: [u8; 32] returned from key extraction also lives on the stack and should be wiped. The simplest approach:

use zeroize::Zeroize;
let mut x25519_priv = private_key_asc_to_x25519_bytes(...)?;
// ... use it ...
x25519_priv.zeroize();

---

Issue 5 — edit doesn't verify the master password

phrase cred edit <alias> calls collect_entry_inputs() and overwrites the entry entirely — no master password prompt. Anyone with shell access to your session can overwrite any vault entry without knowing the master password. It should require the master password first (for authentication), even though re-encryption is correct (fresh session key).

---

Issue 6 — Decrypted files are written to disk

For EntryType::File, display_entry() writes the decrypted file to paths::decrypted_file_path(). That's a plaintext file on disk that persists after the process exits. It should be securely deleted (overwrite with zeros, then unlink) after the user is done with it, using the same Enter-to-clear pattern used for clipboard content.

---

Issue 7 — No SQLCipher / at-rest DB encryption

The SQLite file itself is unencrypted. Vault names, entry aliases, categories, entry types, and UUIDs are all readable with a plain sqlite3 command. For a local-only tool, adding sqlcipher with a key derived from the master password (Argon2id, separate salt stored alongside) would encrypt the entire file at rest. This is optional but meaningfully improves the security posture against casual theft (laptop stolen from sleep, etc.).

---

Summary table

Finding	Severity	Fix
S2K / SHA-256 protects private key at rest	Critical	Switch to standalone X25519 + Argon2id (already implemented in session_key.rs)
Master password / session key not zeroed	High	Wrap in Zeroizing
edit doesn't require master password	Medium	Add password prompt to edit command
Decrypted files written to disk permanently	Medium	Overwrite and delete after use
Alias / category / type stored in plaintext	Low	Encrypt metadata fields with a vault-level key
Ed25519→X25519 cross-key derivation	Low/Design	Consolidate to standalone X25519 keypair
No database encryption at rest	Low	Optional: add SQLCipher

The good news is that issues 1 and 2 have the same root fix: stop using the PGP key pipeline for encryption and use the X25519 + Argon2id path that you already wrote and tested in session_key.rs. That path is cleaner, uses better primitives, and removes the pgp crate dependency from the hot path entirely.

[0xRahim]

Will fix these issues in next update