Skip to content

Security audit: first-pass findings on v7 TC-HKEM vault#16

Draft
johnzfitch wants to merge 4 commits intomainfrom
claude/plan-security-audit-e11PJ
Draft

Security audit: first-pass findings on v7 TC-HKEM vault#16
johnzfitch wants to merge 4 commits intomainfrom
claude/plan-security-audit-e11PJ

Conversation

@johnzfitch
Copy link
Copy Markdown
Owner

Summary

Records the first pass of an insecure-handling audit across the dota crate (~6.5k LoC). Adds SECURITY-AUDIT.md with severity-ranked findings; no source changes. Methodology and per-domain checklist live in the audit plan we discussed; this PR captures the concrete output.

No Critical findings. The v7 TC-HKEM path holds the documented invariants: header HMAC verified before private-key decapsulation, KDF params bounded before Argon2 runs, every commitment compare uses constant_time_eq, min_version rollback floor enforced, all-zero ephemeral keys rejected, vault writes are atomic via tempfile::persist + directory sync_all, and O_NOFOLLOW blocks symlink traversal at the syscall boundary.

High-severity items

  • H1 — README + Cargo.toml advertise a clipboard / ratatui TUI that does not exist. arboard, ratatui, crossterm, and tokio are declared but have zero call sites in src/; tui/app.rs is a 3-line placeholder; dota get and the TUI get write secrets to stdout via println!. Users who follow the README leak secrets into terminal scrollback.
  • H2panic = "abort" in the release profile defeats ZeroizeOnDrop on every panic path; README claim is overstated. Four .expect calls in vault/ops.rs are the real reachable panic surfaces.
  • H3 — Migration backups (MAX_BACKUPS = 5) keep old wrapped key material indefinitely and are never re-encrypted on change-passphrase or rotate-keys. fs::copy followed by chmod is also non-atomic.
  • H4ml-kem = "0.3.0-rc.0" is a release candidate in the cryptographic core; several dead crypto-adjacent deps (see H1); no deny.toml to pin sources/licenses.

Medium / Low items

DOTA_PASSPHRASE env-var support is partial across commands; defense-in-depth gaps in HKDF error paths (okm not zeroized on early return); macOS/Windows have no equivalent of harden_process; secret names are stored plaintext (intentional but undocumented in threat model); plus several documentation drifts. Full list in SECURITY-AUDIT.md.

Suggested PR ordering

H1 → H4 → H3 → H2 → mediums → lows. H3 touches migration.rs, which AGENTS.md flags as "ask first."

Test plan

  • Reviewer reads SECURITY-AUDIT.md end-to-end and decides which findings to accept / dispute / defer.
  • Open follow-up issues (or PRs) for each accepted finding, tagged security-audit-1.
  • Add the regression tests listed in the "Tests to add" section as fixes land.
  • Re-run the static sweeps from the plan after each fix to confirm no new instances appear.

https://claude.ai/code/session_01JsUq2PoX29i1235fPQE2AZ


Generated by Claude Code

Records the output of a static sweep + module-by-module review of the
v7 TC-HKEM vault path. No Critical findings. High items are README/code
drift (clipboard + ratatui dependencies declared but never called),
panic=abort defeating ZeroizeOnDrop, migration backups retaining old
key material, and an RC-version of ml-kem in the cryptographic core.
Medium items cover env-var passphrase inconsistency, defense-in-depth
gaps in HKDF error paths, and platform-specific hardening shortfalls.

https://claude.ai/code/session_01JsUq2PoX29i1235fPQE2AZ
@johnzfitch
Copy link
Copy Markdown
Owner Author

@copilot security review, hardening pass

Copilot AI and others added 3 commits May 7, 2026 07:08
Agent-Logs-Url: https://github.com/johnzfitch/dota/sessions/fba3fcab-4b31-4f53-8567-564d2685170e

Co-authored-by: johnzfitch <10013448+johnzfitch@users.noreply.github.com>
Reformats the multi-line builder calls touched in ca00718 to satisfy
cargo fmt --check. Mechanical formatter-only change; no behavior
difference.

https://claude.ai/code/session_01JsUq2PoX29i1235fPQE2AZ
Agent-Logs-Url: https://github.com/johnzfitch/dota/sessions/fba3fcab-4b31-4f53-8567-564d2685170e

Co-authored-by: johnzfitch <10013448+johnzfitch@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 7, 2026

@copilot security review, hardening pass

Implemented in db57912 (with core hardening in ca00718): release profile now uses panic = "unwind", key-commitment helpers no longer use panic-based expect(...), and errors now preserve underlying crypto context. Validated with cargo test, targeted vault tests, and cargo clippy -D warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants