From 4b64d26273f1a8703edbef33a88c7e52dcf940a3 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN Date: Wed, 19 Aug 2026 09:11:09 -0400 Subject: [PATCH] docs: all seven audit findings closed; record the deferred L-1 v2 design MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixed outright (M-1, M-2, L-4, L-7, L-8), one mitigated with the derivation change deliberately deferred (L-1), two surfaced rather than changed because their resolution is frozen for parity (L-2, L-3), and two resolved as documented-behaviour with the reasoning written down (L-5, L-6). The L-1 v2 design is now written up in TODO.md rather than left as a one-line recommendation: keep v1 frozen, add derivation_version alongside the existing gen_version pattern, domain-separate with an HMAC step rather than new BIP-85 app numbers, land both implementations together with cross-impl fixtures, and make the upgrade opt-in per entry. Sequenced after the outside review on purpose. Also recorded: three defects found while fixing, none of which the audit had reached — Python's publish_delta manifest republish, the fetch-limit downgrade path, and merge_index_payloads mutating its argument. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN --- TODO.md | 67 +++++++++++++++++++++++++++++++++----------- js/SECURITY_AUDIT.md | 33 ++++++++++++++++------ 2 files changed, 76 insertions(+), 24 deletions(-) diff --git a/TODO.md b/TODO.md index c50708e8..babe59e5 100644 --- a/TODO.md +++ b/TODO.md @@ -251,22 +251,57 @@ the review independent in the sense that matters. `js/packages/cli/test/passwordPolicy.test.ts`, four cases against Python-computed values, each also asserted unequal to the pre-fix output; mutation-verified (drop `basePolicy` from the merge and all four go red). -- [ ] **M-1 — restore can return a stale snapshot when two syncs land in the same second.** - Manifests sort by `created_at` (whole-second Nostr resolution) with the event id as - tie-break, which is arbitrary with respect to time, so the older of two same-second - snapshots wins roughly half the time and `nostr restore` silently drops the entries - created between them. Reproduces as the intermittent failure of "syncs the vault to - the relay and restores after local destruction" (~1 in 4 runs). **This one is a - protocol change, not a local fix:** the fix is a monotonic sequence number inside the - signed manifest, used as the secondary sort key, and it has to land in Python at the - same time or the two implementations will disagree about which snapshot is latest. - Decide the wire format before writing code. -- [ ] **L-1 — BIP-85 app-32 path is shared across password, SSH and PGP derivation.** - Protocol-level, both implementations, needs versioning. Same care as M-1. -- [ ] **L-2 / L-3 — merge-level data loss that is silent rather than wrong:** concurrent - same-id creation on two replicas discards one entry at merge; the 2048-entry - tombstone cap allows deletion replay. Both are arguably working as designed; the fix - for each is to surface the event rather than to change the merge. +- [x] **(fixed 2026-08-19) M-1 — restore could return a stale snapshot when two syncs + landed in the same second.** Manifests now carry `published_ms`, a monotonic + publication timestamp inside the signed JSON, and both implementations order by it. + Python additionally stopped delegating "which snapshot is newest" to the relay (it + asked for `limit(1)` and restored whatever came back). Backwards compatible: a + manifest without the field orders at the start of its `created_at` second. The nostr + CLI test went from ~1-in-4 failures to 6/6 clean runs. + **Found while fixing:** Python's `publish_delta` republishes the manifest and needed + the field too (without it the manifest carrying the newest `delta_since` sorted below + the snapshot it superseded, so readers fetched deltas from a stale watermark), and + raising the fetch limit turned a vestigial fallback loop into a live downgrade path — + now it skips relay noise but refuses to reach past the newest manifest that parses. +- [x] **(mitigated 2026-08-19) L-1 — BIP-85 app-32 is shared across password, SSH and PGP.** + Detection shipped in both implementations (`findDerivationCollisions` / + `find_derivation_collisions`), wired into import, `--inspect`, restore's merge, the + TUI, and a new `util check-derivation`. The lead test in each derives the keys and + compares bytes, so the claim is verified rather than asserted. + **The derivation change is deliberately deferred**, see the v2 design below. +- [x] **(surfaced 2026-08-19) L-2 / L-3 — merge-level loss that is silent rather than + wrong.** Both implementations gained an optional `MergeReport`: same-id conflicts + (naming the kept and discarded entries) and a count of tombstones evicted at the + retention cap. `nostr restore` reports both. The resolution itself is unchanged — + it is frozen for parity — and the collector is write-only, asserted in both + implementations, because if observing could change the outcome it would be a + divergence rather than an observation. + **Found while fixing:** Python's `merge_index_payloads` mutated its `current` + argument (shallow copy sharing the nested entries dict), and an existing idempotency + test was passing only because of that aliasing. + +### L-1 v2 derivation: designed, deliberately not yet implemented + +Domain-separate the three app-32 uses so `ssh@N`, `pgp@N` and `password@N` stop +sharing key material. Sketch, for whoever picks this up: + +- Keep v1 exactly as it is, forever — existing SSH keys are on servers and in + forges, existing PGP keys are published. Nothing already derived may change. +- Add `derivation_version` to ssh/pgp entries, absent meaning 1, exactly as + `gen_version` works for passwords. New entries are stamped 2. +- v2 takes the app-32 entropy and applies one HMAC-SHA256 step with a + kind-specific info string (`seedpass:v2:ssh`, `seedpass:v2:pgp`, + `seedpass:v2:password`). Prefer this over new BIP-85 app numbers, which risk + colliding with future registrations. +- Land it in both implementations in one change, with cross-impl fixtures, and + an opt-in per-entry upgrade action — never a bulk migration. The same + "this changes the key; rotate it at the server first" confirmation the + password v2 upgrade needs. + +**Sequencing:** this is a new derivation path, and this project's own rule is +that a derivation bug does not throw — it quietly produces a secret that +cannot be recovered. Adding one immediately before the first outside review +inverts the point of the review. Do it after. - [x] **(fixed 2026-08-18) L-4 — inert settings, and the unported subsystem behind them.** `additional_backup_path` was stored, confirmed by the TUI ("Additional backups will be written to X") and used by nothing — false assurance in a disaster-recovery diff --git a/js/SECURITY_AUDIT.md b/js/SECURITY_AUDIT.md index 7c8e0bf2..c45f964c 100644 --- a/js/SECURITY_AUDIT.md +++ b/js/SECURITY_AUDIT.md @@ -25,7 +25,7 @@ failure is flaky and is the direct evidence for finding M-1. ## Findings -### M-1 (Medium) — Restore can non-deterministically return a stale snapshot when two syncs share a second +### M-1 (Medium) — Restore can non-deterministically return a stale snapshot when two syncs share a second — **FIXED 2026-08-19** - **Where:** `js/packages/core/src/sync/syncFlows.ts:115-117` (`fetchLatestSnapshot` sort) - **What:** Manifests are ordered by `created_at` (Nostr grants only whole-second @@ -83,7 +83,7 @@ failure is flaky and is the direct evidence for finding M-1. pre-fix code produced. Mutation-verified: dropping `basePolicy` from the merge in `secrets.ts` turns all four red. -### L-1 (Low) — BIP-85 app-32 path is shared across password, SSH, and PGP derivation +### L-1 (Low) — BIP-85 app-32 path is shared across password, SSH, and PGP derivation — **MITIGATED 2026-08-19 (detection shipped; derivation change deferred)** - **Where:** `derive/ssh.ts:47-49`, `derive/pgp.ts:171-175`, `derive/password.ts:175` — all three derive from `m/83696968'/32'/{index}'`. @@ -109,7 +109,7 @@ failure is flaky and is the direct evidence for finding M-1. document the password-prefix relationship alongside the existing known limit, and warn on import/merge when two entries of different kinds share an app-32 index. -### L-2 (Low) — Concurrent same-id creation on two replicas silently discards one entry at merge +### L-2 (Low) — Concurrent same-id creation on two replicas silently discards one entry at merge — **SURFACED 2026-08-19** - **Where:** `js/packages/core/src/sync/merge.ts:347-365` (per-id last-writer-wins). - **What:** The allocation watermark is per-replica. Two devices working offline can both @@ -125,7 +125,7 @@ failure is flaky and is the direct evidence for finding M-1. sync/restore summary instead of resolving silently. A protocol-level fix (replica-scoped id ranges or random ids) would need coordination with Python. -### L-3 (Low) — Tombstone retention cap enables deletion replay (documented; confirmed) +### L-3 (Low) — Tombstone retention cap enables deletion replay (documented; confirmed) — **SURFACED 2026-08-19** - **Where:** `merge.ts:17, 405-416` — `TOMBSTONE_RETENTION_CAP = 2048`, oldest evicted. - **What:** After 2048 deletions, older tombstones are dropped; merging a stale replica or @@ -316,11 +316,11 @@ pnpm -r test # core: 234/234 pass (jsdom env: 230 pass, 4 skipped) | Finding | State | Where | |---|---|---| -| M-1 sync tie-break | **open** | needs a signed-manifest sequence number in both implementations | +| M-1 sync tie-break | fixed | signed `published_ms` ordering in both implementations | | M-2 config password policy | fixed | `secrets.ts` `basePolicy`; `test/passwordPolicy.test.ts` | -| L-1 shared BIP-85 app-32 path | open | protocol change, both implementations | -| L-2 concurrent same-id creation | open | surface at merge rather than change it | -| L-3 tombstone replay | open (documented) | as the docs' own roadmap has it | +| L-1 shared BIP-85 app-32 path | mitigated | collision detection in both; derivation change deferred to after review | +| L-2 concurrent same-id creation | surfaced | `MergeReport.conflicts`, reported by `nostr restore` | +| L-3 tombstone replay | surfaced | `MergeReport.tombstonesEvicted` | | L-4 inert settings | fixed | `backups.ts` ports BackupManager's write side; Quick Unlock now says it is unimplemented | | L-5 unanchored `label_regex` | resolved as-is | matches Python's `re.search`; documented in help, `capabilities()`, the parity matrix, and pinned by tests | | L-6 exec allowlist scope | documented | help, `capabilities().tokens.exec_allowlist_semantics`, enforcement-point comment | @@ -331,5 +331,22 @@ Every fix above is mutation-verified: the guard or hook is disabled, the matching tests are confirmed red, and the change is restored. A passing suite was not treated as evidence on its own. +**Every finding in this report is now closed.** Four were fixed outright, one +is mitigated with the derivation change deliberately deferred (adding a new +derivation path immediately before the first outside review inverts the point +of the review), and two are surfaced rather than changed because their +resolution is frozen for cross-implementation parity. + +Three defects were found while fixing them, none of which the audit had +reached: + +- Python's `publish_delta` republished the manifest without the new ordering + field, sorting the manifest carrying the newest `delta_since` below the + snapshot it superseded. +- Raising Python's manifest fetch limit turned a vestigial fallback loop into + a live downgrade path. +- Python's `merge_index_payloads` mutated its `current` argument, and an + existing idempotency test was passing only because of that aliasing. + Still outstanding, and unchanged by this round: no GPT-family review has run. Findings from one model family are one perspective, not independence.