docs: add security audits and Plan 4 for blocker fixes

- 2026-04-18 initial audit verification (all fixed except H8)
- 2026-05-01 audit with 8 new findings (B1-B4, I1-I6)
- Plan 4: Security Blocker Fixes implementation plan

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
adlee-was-taken
2026-05-02 00:42:17 -04:00
parent 8f78b6dc01
commit 71d51c0bea
4 changed files with 1505 additions and 0 deletions

View File

@@ -0,0 +1,158 @@
# Verification: 2026-04-18 Initial Security Audit
**Verified by:** Claude Opus 4.5
**Date:** 2026-05-01
**Methodology:** Code inspection of referenced file paths and line numbers
---
## Summary
| Finding | File Exists | Lines Match Current | Vulnerability Status | Confidence |
|---------|-------------|---------------------|---------------------|------------|
| C1 - Setup web-accessible | ✅ | ❌ refactored | **FIXED** | 10/10 |
| C2 - Message router trusts all | ✅ | ❌ refactored | **FIXED** | 10/10 |
| C3 - Capture innerHTML XSS | ✅ | ❌ refactored | **FIXED** | 10/10 |
| C4 - Autofill no origin check | ✅ | ❌ refactored | **FIXED** | 10/10 |
| H1 - KDF unprefixed concat | ✅ | ❌ refactored | **FIXED** | 10/10 |
| H2 - Master key not zeroized | ✅ | ❌ refactored | **FIXED** | 9/10 |
| H3 - Passphrase gate cosmetic | ✅ | ❌ refactored | **FIXED** | 10/10 |
| H4 - Git shells out unsafely | ✅ | ❌ refactored | **FIXED** | 10/10 |
| H5 - WASM Math.random() | ✅ | ❌ refactored | **FIXED** | 10/10 |
| H6 - Modulo bias | ✅ | ❌ refactored | **FIXED** | 10/10 |
| H7 - rpassword outdated | ✅ | ✅ | **FIXED** | 10/10 |
| H8 - Storage plaintext | ✅ | ⚠️ partial | **ACKNOWLEDGED** | 8/10 |
**Verdict:** All CRITICAL and HIGH findings except H8 have been remediated. The codebase has been significantly refactored since this audit, making line number references obsolete but confirming fixes were applied.
---
## CRITICAL Findings
### C1: Setup wizard web-accessible
- **Original claim:** `web_accessible_resources` with `matches: ["<all_urls>"]` allows any website to inject vault config.
- **Current state:** `extension/manifest.json` line 38 shows `"web_accessible_resources": []` (empty array).
- **Router validation:** `router/index.ts` lines 29-71 verify sender origins (`isPopup`, `isSetup`, `isContent`) and return `unauthorized_sender` for invalid callers.
- **Status:** ✅ **FIXED**
### C2: Service-worker trusts every message
- **Original claim:** `index.ts:116-441` ignores `_sender` and trusts all messages.
- **Current state:** `service-worker/index.ts` is now 100 lines; message handling delegated to modular router.
- **Router checks:**
- `sender.frameId === 0` for content scripts (line 42)
- `sender.id === chrome.runtime.id` (line 43)
- Returns `{ ok: false, error: 'unauthorized_sender' }` for invalid senders
- **Status:** ✅ **FIXED**
### C3: Capture prompt innerHTML injection
- **Original claim:** `capture.ts:172-191` uses innerHTML with attacker-controlled strings in page DOM.
- **Current state:**
- Uses `createShadowHost()` (line 128) for closed Shadow DOM
- Builds DOM via `document.createElement` + `.textContent =` (lines 143-216)
- File header (lines 6-9) documents this pattern
- **Status:** ✅ **FIXED**
### C4: Autofill has no origin check
- **Original claim:** `get_autofill_candidates` accepts URL from message payload; `get_credentials` returns any entry by ID.
- **Current state in `content-callable.ts`:**
- Line 25: `const senderHost = safeHostname(sender.tab?.url ?? '')` — uses sender tab, not message
- Line 44: `if (!itemHost || itemHost !== senderHost) return { ok: false, error: 'origin_mismatch' }`
- Lines 46-51: TOFU origin-ack check before returning credentials
- **Status:** ✅ **FIXED**
---
## HIGH Findings
### H1: Argon2id unprefixed concatenation
- **Original claim:** `crypto.rs:225-227` has `password = passphrase || image_secret` without length prefix.
- **Current state at `crypto.rs:229-236`:**
```rust
let mut password = Zeroizing::new(Vec::with_capacity(8 + nfc_passphrase.len() + 8 + 32));
password.extend_from_slice(&(nfc_passphrase.len() as u64).to_be_bytes());
password.extend_from_slice(&nfc_passphrase);
password.extend_from_slice(&32u64.to_be_bytes());
password.extend_from_slice(image_secret);
```
Also includes NFC normalization (lines 224-227).
- **Status:** ✅ **FIXED**
### H2: Master key never zeroized
- **Original claim:** `Vec<u8>` from `derive_master_key` and intermediates leak into heap.
- **Current state:**
- `crypto.rs:212`: returns `Zeroizing<[u8; 32]>`
- `crypto.rs:232`: password wrapped in `Zeroizing::new()`
- `session.rs` (WASM/CLI): stores keys as `Zeroizing<[u8; 32]>`
- CLI rpassword calls wrapped in `Zeroizing::new()`
- **Note:** JS string zeroization remains a limitation (acknowledged).
- **Status:** ✅ **FIXED**
### H3: Passphrase strength gate cosmetic
- **Original claim:** Extension accepts any non-empty passphrase; CLI only requires 8 chars.
- **Current state:**
- `setup.ts:152,640`: `score < 3` disables button
- `setup.ts:784-789`: server-side re-validation before create
- `generators.rs:124-130`: `validate_passphrase_strength()` requires score >= 3
- **Status:** ✅ **FIXED**
### H4: Git shells out without guards
- **Original claim:** No hooks/gpgsign/editor isolation.
- **Current state in `helpers.rs:41-55`:**
```rust
cmd.args([
"-c", "core.hooksPath=/dev/null",
"-c", "commit.gpgsign=false",
"-c", "core.editor=true",
]);
```
Comment explicitly references "Audit H4".
- **Status:** ✅ **FIXED**
### H5: WASM Math.random()
- **Original claim:** `lib.rs:240-256` uses `Math.random()` for password generation.
- **Current state:** `generate_password` calls `core_generate_password` from relicario-core.
- **generators.rs:**
- Line 6: `use rand::rngs::OsRng;`
- Lines 61-64: Uses `Uniform::from()` with `OsRng`
- No `Math.random()` anywhere in codebase
- **Status:** ✅ **FIXED**
### H6: Modulo bias
- **Original claim:** `main.rs:308-317` uses `% CHARSET.len()`.
- **Current state in `generators.rs`:**
- Line 61: `let dist = Uniform::from(0..charset.len());`
- Line 63: `charset[dist.sample(&mut rng)]` — rejection sampling, no modulo
- **Status:** ✅ **FIXED**
### H7: rpassword 5.0.1 outdated
- **Original claim:** Uses deprecated `prompt_password_stderr`.
- **Current state:** `Cargo.toml` shows `rpassword = "7"`, uses `prompt_password`.
- **Status:** ✅ **FIXED**
### H8: Storage keeps apiToken/imageBase64 plaintext
- **Original claim:** `chrome.storage.local` stores PAT and reference image unencrypted.
- **Current state:** Still true — `popup-only.ts:139-141` stores `vaultConfig` and `imageBase64`.
- **Mitigation:** Acknowledged as design constraint; spec documents that filesystem access to browser profile compromises both factors.
- **Status:** ⚠️ **ACKNOWLEDGED** (not fixed, documented as acceptable tradeoff)
---
## Conclusion
The 2026-04-18 audit identified real vulnerabilities that existed at that time. **All CRITICAL and HIGH findings (C1-C4, H1-H7) have since been remediated** with the exact fixes recommended in the audit. The codebase underwent significant refactoring, making the original line number references obsolete.
H8 remains as an acknowledged design constraint inherent to Chrome extension architecture.
**The audit was accurate and the remediation was thorough.**

View File

@@ -0,0 +1,113 @@
# Verification: 2026-05-01 Security Audit
**Verified by:** Claude Opus 4.5
**Date:** 2026-05-01
**Methodology:** Code inspection of referenced file paths and line numbers
---
## Summary
| Finding | File Exists | Lines Accurate | Vulnerability Real | Confidence |
|---------|-------------|----------------|-------------------|------------|
| 1 - Backup KDF NFC | ✅ | ✅ | ✅ | 10/10 |
| 2 - Commit injection | ✅ | ✅ | ✅ | 10/10 |
| 3 - WASM private key exposure | ✅ | ✅ | ✅ | 10/10 |
| 4 - Test env vars in prod | ✅ | ✅ | ✅ | 10/10 |
| 5 - AttachmentId 64-bit | ✅ | ⚠️ off-by-1 | ✅ | 9/10 |
| 6 - Field history plaintext | ✅ | ✅ | ✅ | 10/10 |
| 7 - Device keys non-functional | ✅ | ✅ | ✅ | 10/10 |
| 8 - Path traversal restore | ✅ | ✅ | ✅ | 10/10 |
**Verdict:** All 8 findings are verified as real vulnerabilities in the current codebase.
---
## Finding-by-Finding Verification
### Finding 1 — Backup KDF missing NFC normalization
- **File:** `crates/relicario-core/src/backup.rs`
- **Claimed lines:** 303-312
- **Verified:** ✅ `derive_backup_key` at lines 303-312 passes `passphrase` directly to `argon.hash_password_into()` without NFC normalization. Compare to `derive_master_key` in `crypto.rs:224-227` which explicitly normalizes.
- **Impact confirmed:** Cross-platform restore failure for non-ASCII passphrases.
### Finding 2 — Commit message injection via item titles
- **File:** `crates/relicario-cli/src/main.rs`
- **Claimed lines:** 565, 899-901, 1110, 1327
- **Verified:** ✅
- Line 565: `format!("add: {} ({})", item.title, item.id.as_str())`
- Line 1110: `format!("edit: {} ({})", item.title, item.id.as_str())`
- Line 1327: `format!("trash: {} ({})", item.title, item.id.as_str())`
- **Impact confirmed:** Newlines/control chars in titles corrupt git log output.
### Finding 3 — WASM `generate_device_keypair` crosses private key to JS
- **File:** `crates/relicario-wasm/src/lib.rs`
- **Claimed lines:** 215-227
- **Verified:** ✅ Function returns `{ "private_key_base64": "..." }` as `JsValue`, exposing ed25519 private key to JavaScript heap.
- **Impact confirmed:** Key material accessible to any JS in service worker context.
### Finding 4 — Test env vars ship in production binary
- **File:** `crates/relicario-cli/src/main.rs`
- **Claimed lines:** 445-446, 421-423, 1425-1426
- **Verified:** ✅
- Lines 421-423: `RELICARIO_TEST_ITEM_SECRET`
- Lines 445-446: `RELICARIO_TEST_PASSPHRASE`
- Lines 1425-1426: `RELICARIO_TEST_BACKUP_PASSPHRASE`
- **Impact confirmed:** All checked in production code without `#[cfg(test)]`. Passphrase visible in `/proc/<pid>/environ`.
### Finding 5 — `AttachmentId` truncated to 64 bits
- **File:** `crates/relicario-core/src/ids.rs`
- **Claimed lines:** 52-57
- **Actual lines:** 51-56 (off by 1)
- **Verified:** ✅ `&digest[..8]` = 8 bytes = 64 bits. Birthday collision at ~2³² work.
- **Impact confirmed:** Attacker with attachment upload can cause silent overwrites.
### Finding 6 — `get_field_history` returns plaintext to JS
- **File:** `crates/relicario-wasm/src/lib.rs`
- **Claimed lines:** 232-265
- **Verified:** ✅ Returns historical `Password`/`Concealed` values as plaintext JSON via `v.as_str().to_owned()`.
- **Impact confirmed:** Password history exposed to JS heap without Zeroizing.
### Finding 7 — Device key system is security theater
- **File:** `crates/relicario-cli/src/main.rs`
- **Claimed lines:** 2151-2221
- **Verified:** ✅ `cmd_device()` handles Add/List/Revoke but:
- No `sign_commit` or `verify_signature` functions exist anywhere
- `devices.json` is plaintext and unauthenticated
- Revocation has no enforcement mechanism
- **Impact confirmed:** Users falsely believe device revocation provides security.
### Finding 8 — Path traversal on backup restore
- **File:** `crates/relicario-cli/src/main.rs`
- **Claimed lines:** 1619-1626
- **Verified:** ✅
```rust
for item in &unpacked.items {
fs::write(target.join("items").join(format!("{}.enc", item.id)), ...)?;
}
```
`item.id` and `attachment_id` used directly in path construction with no validation.
- **Impact confirmed:** Crafted `.relbak` with `id = "../../.bashrc"` escapes target directory.
---
## Blockers Assessment
The audit's "Path to Certifiable Safety" section is accurate:
| Blocker | Verified | Severity |
|---------|----------|----------|
| B1 - Device key theater | ✅ Real | High |
| B2 - Backup KDF NFC | ✅ Real | Medium |
| B3 - Test env vars | ✅ Real | Medium |
| B4 - Path traversal | ✅ Real | Medium |
All four blockers are confirmed. B1 is the most dangerous as it misleads users about their security posture.

View File

@@ -0,0 +1,199 @@
# Relicario Security Audit — 2026-05-01
Scope: full project audit (not a PR diff). Covers crypto correctness, protocol gaps,
implementation reality vs. plans, and a roadmap toward third-party auditability.
---
## Section 1 — Security Findings
### Finding 1 — Backup KDF missing NFC normalization
**`crates/relicario-core/src/backup.rs:303-312`** · Severity: **Medium**
`derive_backup_key` passes raw passphrase bytes to Argon2id. The main vault KDF in
`crypto.rs` uses `u64_be(len) || nfc_passphrase || u64_be(32) || image_secret`. The
backup KDF has neither NFC normalization nor the length-prefix construction.
**Exploit:** User creates a backup on macOS (NFD normalization) and restores on Linux
(NFC). The Argon2id input differs → wrong key → unrestorable backup. Affects any
non-ASCII passphrase (`"Crêpe-7"`, `"café"`, accented chars).
**Fix:** Factor out `normalize_passphrase()` and use it in both `derive_master_key` and
`derive_backup_key`.
---
### Finding 2 — Commit message injection via item titles
**`crates/relicario-cli/src/main.rs:565, 899-901, 1110, 1327`** · Severity: **Medium**
Item titles (arbitrary user UTF-8) are embedded directly into `-m` commit message strings
via `format!("add: {} ({})", item.title, ...)`. `git_command` uses `Command::args()` (no
shell), so shell injection into `git add` is blocked — but newlines in titles produce
malformed multi-line commit messages that corrupt git log parsers.
**Fix:** Strip control characters from titles before embedding in commit messages, or omit
the title from the `-m` format entirely and use only the item ID.
---
### Finding 3 — WASM `generate_device_keypair` crosses private key bytes to JS
**`crates/relicario-wasm/src/lib.rs:215-227`** · Severity: **Medium**
Returns `{ "private_key_base64": "..." }` as a `JsValue`. The ed25519 private key lives in
the JS heap with no `Zeroizing` protection. The vault master key is protected behind an
opaque `SessionHandle` and never crosses to JS — the device key has no such protection.
**Exploit:** Any JS running in the extension service worker context (compromised dependency,
content script escalation) that can intercept the return value gets the raw device key.
**Fix:** Never return the private key to JS. Expose only a `sign(handle, data) → signature`
API; perform the signing in Rust.
---
### Finding 4 — Test env vars ship in production binary
**`crates/relicario-cli/src/main.rs:445-446, 421-423, 1425-1426`** · Severity: **Medium**
`RELICARIO_TEST_PASSPHRASE`, `RELICARIO_TEST_ITEM_SECRET`, `RELICARIO_TEST_BACKUP_PASSPHRASE`
are checked in production code (not `#[cfg(test)]`). When set, they bypass the interactive
TTY prompt.
**Exploit:** On Linux, `/proc/<pid>/environ` exposes the passphrase in cleartext to
same-UID processes. Shell history captures `RELICARIO_TEST_PASSPHRASE=mysecret relicario unlock ...`.
**Fix:** Gate behind `#[cfg(test)]` or a `--features testing` build profile.
---
### Finding 5 — `AttachmentId` truncated to 64 bits of SHA-256
**`crates/relicario-core/src/ids.rs:52-57`** · Severity: **Medium**
`AttachmentId::from_plaintext` takes `&digest[..8]` (8 bytes = 64 bits). Standard
content-addressed stores use ≥128 bits. With 64 bits, an attacker who can supply attachment
content can find a second-preimage collision with ~2^32 work, causing a crafted attachment
to silently overwrite an existing one on disk.
**Fix:** Change `&digest[..8]``&digest[..16]` (128 bits). No migration needed for
existing vaults since only new attachments are affected.
---
### Finding 6 — `get_field_history` re-parses item JSON from JS heap
**`crates/relicario-wasm/src/lib.rs:232-265`** · Severity: **Medium**
Returns all historical `Password`/`Concealed` values as plaintext `JsValue`. The values
are regular `String` allocations with no `Zeroizing` wrapper before serialization into
`serde_json::Value`.
**Fix:** Architectural — document that the caller must treat the return value as sensitive.
For strong hygiene: do all history display in Rust, never returning password history bytes
to JS.
---
### Finding 7 — Device key system is non-functional as a security control
**`crates/relicario-cli/src/main.rs:2151-2221`** · Severity: **High**
`device add/list/revoke` and `generate_device_keypair` exist, but **no code anywhere signs
git commits with device keys**, and **no code verifies device signatures**. `devices.json`
is plaintext in the repo and unauthenticated by the vault.
**Exploit:** Users believe "device revocation" prevents unauthorized access after a device
is stolen/compromised. It does nothing. A stolen device continues to have full vault access
via its git remote credentials regardless of revocation.
**Fix:** Either (a) implement commit signing + server-side pre-receive hook verification, or
(b) remove the `device` subcommands and document that access control is SSH-key-level only.
---
### Finding 8 — Path traversal on backup restore
**`crates/relicario-cli/src/main.rs:1619-1626`** · Severity: **Medium**
During restore, item/attachment IDs from the decrypted backup JSON are used directly as
path components with no format validation. IDs are AEAD-authenticated but a user restoring
from a crafted `.relbak` with a known passphrase would execute arbitrary path writes.
**Exploit (social engineering):** Attacker provides a `.relbak` with item ID
`../../.bashrc` → restore overwrites `~/.bashrc`.
**Fix:**
```rust
ensure!(id.len() == 16 && id.chars().all(|c| c.is_ascii_hexdigit()),
"invalid id in backup");
```
---
## Section 2 — Implementation Status
| Feature | Status | Notes |
|---|---|---|
| Two-factor decrypt (passphrase + image_secret) | ✅ Implemented | Full crypto pipeline, NFC passphrase, Argon2id m=64MiB t=3 p=4 |
| imgsecret embed | ✅ Implemented | DCT QIM, QUANT_STEP=50, central 70%, 5-50 redundant copies |
| imgsecret extract + crop recovery | ✅ Implemented | Majority voting ≥60%, 4 crop search strategies |
| Manifest browse (schema v2) | ✅ Implemented | Encrypted with master key, search(), title/type/tags/icon |
| Vault CRUD (init/add/edit/rm/trash/restore/purge) | ✅ Implemented | All 7 item types fully handled |
| CLI `init` | ✅ Implemented | zxcvbn ≥3 gate, image embed, Argon2id params, git init |
| CLI `add` / `edit` | ✅ Implemented | All 7 types, TOTP QR decode via rqrr, field history capture |
| CLI `generate` | ✅ Implemented | Random (rejection-sampled) + BIP39, uses vault defaults |
| CLI `sync` | ✅ Implemented | `git pull --rebase && git push` |
| CLI `backup export/restore` | ✅ Implemented | Plan 3A: zstd+AEAD container, optional image + git bundle |
| CLI `import lastpass` | ✅ Implemented | Plan 3B: CSV validation, Login + SecureNote + TOTP mapping |
| WASM bindings (all item/manifest/settings) | ✅ Implemented | Complete symmetric set |
| WASM session handle (opaque master key) | ✅ Implemented | Key never crosses WASM boundary |
| WASM attachment, generator, TOTP, backup, import | ✅ Implemented | All wired |
| Field history tracking + CLI `history` | ✅ Implemented | Password/Concealed/TOTP history, prune policies |
| Trash + retention | ✅ Implemented | `trash list/empty`, TrashRetention window |
| Attachments (CLI + WASM) | ✅ Implemented | File-level AEAD, cap enforcement, Document type |
| Settings / VaultSettings | ✅ Implemented | All retention + generator + cap fields, CLI subcommands |
| Device keys (add/list/revoke) | ⚠️ Partial | Key gen + persistence only — **no signing, no verification** (Finding 7) |
| Per-vault total attachment cap | ⚠️ Partial | Cap defined in settings, per-attachment enforced — per-vault total bytes not checked |
| Browser extension UI | ⚠️ Partial | WASM surface complete; extension TypeScript/HTML is a separate repo |
| Recovery QR | ❌ Plan-only | Spec written; no `recovery_qr.rs` module exists |
| Password coloring | ❌ Plan-only | Spec written; no implementation |
| Passphrase rotation | ❌ Deferred | Explicitly back-burnered |
| Pre-v0.3.0 audit walk | ❌ Not started | Listed as pending before v0.3.0 tag |
| HOTP counter persistence | ❌ Bug | `Hotp { counter }` never incremented/saved — HOTP desynchronizes immediately |
---
## Section 3 — Path to Certifiable Safety
### Blockers — must fix before any real use
| # | Item |
|---|---|
| B1 | **Device key system is security theater** — implement signing or remove the commands. This is the most dangerous finding because it misleads users about their security posture. |
| B2 | **Backup KDF NFC normalization** — one-line fix; data loss risk for non-ASCII passphrases. |
| B3 | **Test env vars in production binary** — gate with `#[cfg(test)]`. Exposes passphrase via `/proc`. |
| B4 | **Path traversal on restore** — two-line ID validation before any `fs::write`. |
### Important — fix before third-party audit
| # | Item |
|---|---|
| I1 | Sanitize item titles before embedding in commit messages |
| I2 | `AttachmentId`: `&digest[..8]``&digest[..16]` (128-bit collision resistance) |
| I3 | Enforce per-vault total attachment bytes cap (already defined, never checked) |
| I4 | Document manifest integrity model: AEAD protects against silent modification, but item deletion is only detectable via git history |
| I5 | Stop crossing device private key bytes to JS (prerequisite for B1 if signing is implemented) |
| I6 | Fix HOTP counter: increment + re-save on each `totp get`, or disable HOTP and return an error |
### Nice-to-have — audit-friendliness
| # | Item |
|---|---|
| N1 | Wrap `nfc_passphrase: Vec<u8>` in `Zeroizing` in `derive_master_key` |
| N2 | `cargo audit` in CI |
| N3 | Validate Argon2id params on vault load — warn if below production minimums |
| N4 | Broaden steganography recompression tests to use ImageMagick/libjpeg-turbo (not just the `image` crate) |
| N5 | Consider machine-readable audit log encrypted alongside the vault |