mirror of
https://github.com/PR0M3TH3AN/SeedPass.git
synced 2026-10-27 07:52:03 +00:00
docs: independent security audit of the TypeScript port at e9d8908
Full manual review of js/packages/core and js/packages/cli plus supply chain. No critical or high findings. Two mediums: a same-second snapshot tie-break that lets a restore return stale state (the flaky nostr CLI test is this bug), and config-level password policy being ignored in TS derivation, diverging from Python. Lows and informational notes cover the app-32 path sharing, inert settings, token regex anchoring, and plaintext-import fingerprint binding. Co-Authored-By: Claude Fable 5 <[email protected]>
This commit is contained in:
thePR0M3TH3ANandClaude Fable 5 committed
1 parent
e9d8908560
commit
f3c3d73c3d
1 file changed
+298
@@ -0,0 +1,298 @@
|
||||
# SeedPass TypeScript Port — Security Audit
|
||||
|
||||
- **Date:** 2026-08-18
|
||||
- **Auditor:** Claude (Fable 5), full manual source review
|
||||
- **Scope:** the TypeScript port only — `js/packages/core`, `js/packages/cli`, build/CI supply chain
|
||||
- **Commit audited:** `e9d8908` (`port/typescript-web-extension`), clean working tree
|
||||
- **Method:** line-by-line read of every non-test source file (~7,400 lines), cross-checked
|
||||
against the Python reference where parity claims matter, plus a full test-suite run as a
|
||||
baseline probe. No edits were made to the repository other than this report.
|
||||
|
||||
## Verdict
|
||||
|
||||
**No critical or high-severity vulnerabilities were found.** The cryptographic core, the
|
||||
agent trust boundary, the relay sync transport, and the filesystem layer are unusually
|
||||
well-engineered, with prior audit cycles visibly absorbed into the code. The findings below
|
||||
are two medium-severity correctness/data-loss issues (one with a reproducible failing test),
|
||||
a cluster of low-severity hardening and parity gaps, and informational notes. Nothing found
|
||||
here allows secret extraction by an attacker who does not already hold the seed, the master
|
||||
password, or same-uid code execution (the documented trust boundary).
|
||||
|
||||
Baseline at this commit: `pnpm -r typecheck` clean; **391 of 392 tests pass**. The single
|
||||
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
|
||||
|
||||
- **Where:** `js/packages/core/src/sync/syncFlows.ts:115-117` (`fetchLatestSnapshot` sort)
|
||||
- **What:** Manifests are ordered by `created_at` (Nostr grants only whole-second
|
||||
resolution) with the event id as tie-break. The id tie-break is stable and
|
||||
relay-independent — but it is *arbitrary with respect to time*. Two snapshots published
|
||||
within the same second tie on `created_at`, and the older one wins the id comparison
|
||||
roughly half the time. `nostr restore` then silently restores the older vault state;
|
||||
entries created between the two syncs disappear with no warning
|
||||
(`skippedNewerManifests` stays empty because nothing was *skipped* — the wrong manifest
|
||||
simply sorted first).
|
||||
- **Evidence:** `js/packages/cli/test/nostr.test.ts` — "syncs the vault to the relay and
|
||||
restores after local destruction" fails intermittently (reproduced 1 in 4 runs on this
|
||||
machine: the restore returned only the earlier test's `pre-sync` entry). This flake is
|
||||
not the environment; it is the bug.
|
||||
- **Impact:** Silent loss of recent entries on restore. Most likely in scripted or
|
||||
automated sync flows that publish twice quickly; also gives a relay a small window to
|
||||
serve the older of two same-second snapshots with full deniability.
|
||||
- **Recommendation:** Add a monotonic sequence number inside the (signed) manifest JSON and
|
||||
use it as the secondary sort key; alternatively detect a `created_at` tie among
|
||||
assemblable manifests and either merge both or surface the ambiguity to the caller. The
|
||||
fix must land on the Python side too, or the two implementations will disagree on which
|
||||
snapshot is "latest."
|
||||
|
||||
### M-2 (Medium) — Config-level password policy is ignored; same entry derives different passwords in TS vs Python
|
||||
|
||||
- **Where:** `js/packages/core/src/derive/password.ts:53-64` (`resolvePolicy` hardcodes
|
||||
defaults); `js/packages/cli/src/secrets.ts:43-49` (only the entry's own `policy` block is
|
||||
passed).
|
||||
- **What:** Python builds its `PasswordGenerator` with
|
||||
`policy=self.config_manager.get_password_policy()` (`src/seedpass/core/manager.py:2033`)
|
||||
and merges the entry's `policy` block *over that config base*
|
||||
(`manager.py:3167-3201`). The TS port merges the entry block over hardcoded defaults
|
||||
(`min_* = 2`, specials on). The per-profile config keys (`min_uppercase`, `min_digits`,
|
||||
…) exist in the TS config (`configFile.ts` mirrors them) but are never read by
|
||||
derivation.
|
||||
- **Impact:** Any Python profile whose config policy differs from the defaults derives
|
||||
**different passwords** in the TS port for entries that carry no explicit policy block.
|
||||
To the user this presents as wrong passwords / apparent data loss after migration. Users
|
||||
with default config are unaffected.
|
||||
- **Recommendation:** Thread the profile config's policy fields into
|
||||
`materializeSecret`/`generatePassword` as the base policy before applying the entry's
|
||||
overrides, and add a cross-implementation fixture with a non-default config policy —
|
||||
the current parity suite does not cover this case, which is why it passed.
|
||||
|
||||
### L-1 (Low) — BIP-85 app-32 path is shared across password, SSH, and PGP derivation
|
||||
|
||||
- **Where:** `derive/ssh.ts:47-49`, `derive/pgp.ts:171-175`, `derive/password.ts:175` — all
|
||||
three derive from `m/83696968'/32'/{index}'`.
|
||||
- **What (verified end-to-end):**
|
||||
- `ssh@N` and `pgp@N` take the first 32 bytes of the same HMAC-SHA512 digest → **the
|
||||
same Ed25519 private key**, byte for byte.
|
||||
- `password@N` takes 64 bytes of that digest → the same-index SSH/PGP private key **is
|
||||
the first half of the password's input entropy** (the password itself remains safe:
|
||||
256 unknown bits survive, and PBKDF2 is one-way — but this violates key-separation
|
||||
hygiene, and SSH keys are exactly the kind of secret that gets exported and used).
|
||||
- **Reachability:** The TS creation path cannot produce the collision — an entry's
|
||||
derivation index equals its vault id (`entryOps.ts:270/275, 418/423`), ids are unique
|
||||
(`insert` throws on overwrite), and the persisted watermark (`e3b1325`) prevents id
|
||||
reuse. It **is** reachable through data: the schema accepts any nonnegative `index` on
|
||||
ssh/pgp entries (`schema/entries.ts:78-106`), so Python-created vaults, imported
|
||||
backups, and sync merges can carry `ssh@N` and `pgp@N` with equal indices, and the TS
|
||||
port will faithfully derive identical keys.
|
||||
- **Status:** The ssh/pgp collision is a documented known limit shared with Python
|
||||
(`docs/agent_security_model.md`). The password-entropy-prefix relationship extends that
|
||||
documented finding and does not appear in the docs.
|
||||
- **Recommendation:** Protocol-level fix (both implementations, versioned): domain-separate
|
||||
the three uses — different app numbers or an HKDF step keyed by entry kind. Short term:
|
||||
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
|
||||
|
||||
- **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
|
||||
allocate id N; the merge resolves per id by timestamp-then-hash, and the losing entry is
|
||||
silently replaced — no conflict is recorded or surfaced. Because an id is a permanent
|
||||
BIP-85 derivation coordinate, the surviving entry of a different kind may derive related
|
||||
key material (see L-1).
|
||||
- **Impact:** Silent entry loss under a realistic multi-device workflow. Inherent to the
|
||||
deterministic-CRDT design and matched by Python (parity-frozen), so this is a design
|
||||
finding, not a divergence.
|
||||
- **Recommendation:** Surface it: the merge already computes everything needed to detect
|
||||
"both sides created different-kind entries at the same id" — report those ids in the
|
||||
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)
|
||||
|
||||
- **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
|
||||
an old relay snapshot then resurrects entries the user deleted. Documented as a known
|
||||
protocol limit shared with Python; confirmed present and correctly implemented as
|
||||
specified.
|
||||
- **Recommendation:** None beyond the docs' own roadmap; consider counting evictions and
|
||||
warning the user when the cap has actually trimmed history.
|
||||
|
||||
### L-4 (Low) — Inert settings: the TUI confirms behavior that does not exist
|
||||
|
||||
- **Where:** `tui/menus.ts:1482-1489` (`additional_backup_path` — the TUI replies
|
||||
"Additional backups will be written to X" but no code path ever writes a backup there);
|
||||
`tui/menus.ts:1012` (`quick_unlock_enabled` — a toggle with no effect); also stored but
|
||||
unused: `backup_interval`, `pin_hash`, `nostr_max_retries`, `nostr_retry_delay`
|
||||
(`configFile.ts:26-47`).
|
||||
- **Impact:** `additional_backup_path` is the harmful one: a user can reasonably believe a
|
||||
second copy of their vault exists when it does not — false assurance in a disaster-
|
||||
recovery feature. The rest are cosmetic.
|
||||
- **Note:** This class of bug is known here — commits `135a626`, `933cad3`, `a698255`
|
||||
fixed three other inert settings. These are the stragglers.
|
||||
- **Recommendation:** Either implement them or have the menu say plainly "not available in
|
||||
this build," the pattern already used for QR codes and the semantic index.
|
||||
|
||||
### L-5 (Low) — Token `label_regex` is unanchored: substring semantics widen token scope
|
||||
|
||||
- **Where:** `agent.ts:248-255` (`tokenMaySee` uses bare `RegExp.test`); surfaced at
|
||||
`program.ts:1318` (`--label-regex`).
|
||||
- **What:** A token issued with `--label-regex prod` matches every label *containing*
|
||||
"prod" — including `not-prod-db`. The operator issuing the token is the one most likely
|
||||
to be surprised. (Owner-supplied pathological regexes are also unbounded, but the owner
|
||||
can only ReDoS their own daemon — noted, not scored.)
|
||||
- **Recommendation:** Anchor the pattern (`^(?:${re})$`) or document substring semantics
|
||||
loudly in `--label-regex` help and `capabilities()`. Match Python's behavior, whichever
|
||||
it is, and add it to the parity matrix.
|
||||
|
||||
### L-6 (Low) — Exec allowlist constrains the command word, not its arguments
|
||||
|
||||
- **Where:** `agent.ts:337-349`; execution in `sinks.ts:102-121`.
|
||||
- **What:** `exec_allowlist` compares only the first token. A use-scoped holder may pass
|
||||
arbitrary arguments to an allowlisted binary. Sink children inherit the *daemon's*
|
||||
stdio, not the caller's, and get a scrubbed env — good — but the child's 1-bit exit code
|
||||
is returned per use, and an argument-flexible binary (anything with an output-file or
|
||||
network flag) can move `SEEDPASS_SECRET` somewhere the token holder can read.
|
||||
- **Status:** The docs already state "use can become reveal without an exec allowlist";
|
||||
this finding is that the allowlist's containment is weaker than an operator would
|
||||
assume even *with* it.
|
||||
- **Recommendation:** Document that allowlisted binaries must be chosen as if the token
|
||||
holder controls their argv; longer term, allow full argv templates in the allowlist
|
||||
(`["ssh-add", "-"]`) rather than bare command words.
|
||||
|
||||
### L-7 (Low) — Daemon uses wire-supplied `fingerprint` as a path component without format validation
|
||||
|
||||
- **Where:** `agent.ts:209, 298, 554` (`join(appDir, fingerprint, ...)`); `put` at
|
||||
`agent.ts:633-652` accepts any string.
|
||||
- **What:** The CLI layer validates fingerprints (`appDir.ts:82-92`,
|
||||
`/^[0-9A-F]{16}$/`) before they touch a path; the daemon does not. Every reaching path
|
||||
first requires the fingerprint to be in `held`, and only the owner-gated `put` populates
|
||||
`held` — so exploitation requires the owner capability, i.e. an attacker who has already
|
||||
won under the documented same-uid model. Defense-in-depth gap only.
|
||||
- **Recommendation:** Call `assertValidFingerprint` in the daemon's `put` handler and also
|
||||
verify `generateFingerprint(mnemonic) === fingerprint`, which additionally catches an
|
||||
honest mismatch corrupting the audit-log location.
|
||||
|
||||
### L-8 (Low) — Plaintext backup import is not bound to a profile
|
||||
|
||||
- **Where:** `program.ts:1549-1606` (`vault import`), `tui/menus.ts:1432-1459`
|
||||
(`importDatabase`); wrapper schema in `core/src/vault/portableBackup.ts`.
|
||||
- **What:** Encrypted (`seed-only`) backups are implicitly seed-bound — decryption fails
|
||||
under the wrong profile. Plaintext (`encryption_mode: "none"`) backups carry a
|
||||
`fingerprint` field that is **never compared** to the target profile. A plaintext backup
|
||||
of vault A imports cleanly into profile B; every derived entry (passwords, SSH, PGP,
|
||||
seeds) then silently produces *different* secrets than the originals.
|
||||
- **Also:** the CLI detects encryption mode with a regex over the raw file text
|
||||
(`program.ts:1560`) rather than parsing the JSON. It is not exploitable (JSON string
|
||||
escaping prevents a false match), but it is fragile; parse the wrapper instead.
|
||||
- **Recommendation:** Compare `wrapper.fingerprint` to the target profile's fingerprint
|
||||
and require an explicit override on mismatch, in both the CLI and TUI paths.
|
||||
|
||||
---
|
||||
|
||||
## Informational notes
|
||||
|
||||
- **I-1 — Agent socket framing decodes chunks independently.** `agent.ts:699-723` builds
|
||||
the line buffer with per-chunk `toString("utf8")`; a multibyte UTF-8 character split
|
||||
across TCP chunks corrupts that request (non-ASCII labels are legal). Use
|
||||
`string_decoder`. Separately, pipelined requests on one connection get replies in
|
||||
completion order with no request ids; the bundled `AgentClient` opens one connection per
|
||||
request and is unaffected, but a future client (the planned browser extension) could
|
||||
misattribute replies. Consider serializing per-connection handling or adding ids.
|
||||
- **I-2 — v1 password generation has modulo bias** (`password.ts:179-183`,
|
||||
`byte % alphabet.length` over a ~94-char alphabet). Parity-frozen forever by design; v2
|
||||
fixed it with rejection sampling. Worth stating in the security docs so it is a known
|
||||
property, not a rediscovery.
|
||||
- **I-3 — Fingerprint normalization is stronger than Python's.** `fingerprint.ts` hashes
|
||||
the fully canonicalized mnemonic (NFKD, internal whitespace collapsed); Python's
|
||||
`fingerprint.py` uses `strip().lower()` only. A phrase with doubled internal spaces
|
||||
produces *different profile identities* across implementations. Edge case; add a parity
|
||||
fixture or align Python.
|
||||
- **I-4 — `token-issue` coerces `name` and `label_regex` with `String()`**
|
||||
(`agent.ts:498, 503`) while other wire fields are shape-checked; an array arrives as
|
||||
`"a,b"`. Harmless today; shape-check for consistency.
|
||||
- **I-5 — Token lookup compares secret hashes non-constant-time** (`agent.ts:233-236`).
|
||||
Comparing SHA-256 digests of a high-entropy secret; not practically exploitable. The
|
||||
owner capability check does use `timingSafeEqual`.
|
||||
- **I-6 — TOTP index derivation from imported data:** a malformed `index` on an existing
|
||||
totp entry makes `nextTotpIndex` return NaN and the subsequent derivation throw
|
||||
(`entryOps.ts:117-123`) — a loud failure, not a corruption; noted only.
|
||||
|
||||
---
|
||||
|
||||
## What was checked and found sound
|
||||
|
||||
The following areas were read in full and are called out because they are load-bearing —
|
||||
and correct:
|
||||
|
||||
- **AEAD/vault crypto** (`aead.ts`, `payload.ts`, `fernet.ts`): AES-256-GCM with fresh
|
||||
random 12-byte nonces per call; V2/legacy Fernet decrypt-only with HMAC verified before
|
||||
decryption using a constant-time compare (which also forecloses a CBC padding oracle);
|
||||
format dispatch mirrors Python exactly.
|
||||
- **Key derivation** (`indexKey.ts`, `passwordKdf.ts`, `bip85.ts`): HKDF-SHA256 with
|
||||
domain separation; KDF parameters read from disk are bounded (`KDF_LIMITS`) so a
|
||||
tampered kdf block cannot hang the process or allocate gigabytes; mnemonics are
|
||||
canonicalized before both validation and derivation; profile KDF default is 200k
|
||||
PBKDF2 iterations matching Python (an earlier-session concern about a 100k default was
|
||||
wrong — 100k is only a legacy *fallback candidate* on decrypt).
|
||||
- **Agent daemon** (`agent.ts`): owner ops gated on a capability file outside the app dir
|
||||
(0600, `wx`, under `XDG_RUNTIME_DIR`); token auth happens *before* vault decryption
|
||||
(closes an enumeration oracle and a CPU-burn probe); missing entry vs. constraint denial
|
||||
are indistinguishable; `Object.hasOwn` blocks `__proto__` lookups; TOTP timestamps are
|
||||
clamped to ±120 s for token callers; NaN-TTL coercion rejected; connection and
|
||||
line-length caps; socket created under a restrictive umask before chmod.
|
||||
- **Audit log** (`audit.ts`): HMAC chain plus a signed length-pinning head, with exactly
|
||||
one crash-skew record tolerated; a missing or mismatched head fails both verification
|
||||
and *append*, so truncation cannot be laundered. Honest limits documented in-file.
|
||||
- **Sinks** (`sinks.ts`): secrets via env or stdin, never argv; child env is an allowlist
|
||||
(secrets, tokens, and socket paths do not leak into sink children); clipboard auto-clear
|
||||
compares before wiping; EPIPE from an early-exiting child cannot crash the daemon.
|
||||
- **Relay sync transport** (`events.ts`, `relay.ts`, `snapshot.ts`): every fetched event
|
||||
is re-verified against the *requested filter* (authors pinned to the profile's own
|
||||
pubkey — relays are not trusted to honor filters), signature-verified, well-formedness
|
||||
checked with correct `created_at` type discipline; decompression bombs are killed
|
||||
mid-stream against explicit byte budgets; relay downgrade (withholding a chunk) is
|
||||
detected and reported via `skippedNewerManifests`; delta replay across snapshot lineages
|
||||
is blocked by client-side `#e`-tag re-checking.
|
||||
- **Merge determinism** (`canonical.ts`, `merge.ts`): CPython-exact canonical JSON
|
||||
(ensure_ascii, float repr, code-point key sort) with loud rejection of unsafe integers;
|
||||
null-prototype maps make `__proto__` an ordinary key on both entries and tombstones.
|
||||
- **Filesystem discipline** (`vaultFile.ts`, `appDir.ts`): atomic writes via `wx` temp +
|
||||
fsync + rename + directory fsync, short-write loop, fchmod-on-handle (not path);
|
||||
fingerprints validated before becoming path components; recursive profile delete
|
||||
re-verifies containment; lock files carry owner tokens and are stolen only from
|
||||
demonstrably dead pids.
|
||||
- **CLI egress policy** (`program.ts`, `refs.ts`, TUI): reference-first output with an
|
||||
*allowlist* metadata redactor (unknown fields become `has_*` flags — the right polarity
|
||||
for loose schemas); plaintext egress is confined to explicitly named commands; token
|
||||
mode cannot escalate to owner (`SEEDPASS_TOKEN` blocks mnemonic resolution); new-seed
|
||||
generation refuses to write a phrase into a pipe; TUI database export is always
|
||||
encrypted (plaintext export exists only behind the CLI's explicit `--plaintext`);
|
||||
secret-bearing file writes go through fresh-0600-inode atomic writes with overwrite
|
||||
confirmation.
|
||||
- **Supply chain**: runtime deps are exactly noble/scure crypto + commander + zod, pinned
|
||||
by a lockfile with integrity hashes and installed `--frozen-lockfile` in CI; CI runs
|
||||
typecheck, tests, a standalone-bundle smoke test, an npm-pack install test, `pnpm audit`,
|
||||
and generates a CycloneDX SBOM; the shipped artifact is a single non-minified esbuild
|
||||
bundle with a SHA-256 sidecar; official BIP-85 spec vectors run directly against the
|
||||
core (`ecff57e`).
|
||||
|
||||
## Known limits (documented, verified to match the code)
|
||||
|
||||
Per `docs/agent_security_model.md`, and confirmed accurate against this commit: a same-uid
|
||||
attacker wins (capability file, ptrace); seeds/secrets are not scrubbed from JS memory;
|
||||
`use` becomes `reveal` without an exec allowlist (and see L-6 for the allowlist's own
|
||||
limits); the capability file exists on disk for the daemon's lifetime; tombstone replay
|
||||
(L-3) and the ssh/pgp index collision (L-1) are shared with Python.
|
||||
|
||||
## Baseline
|
||||
|
||||
```
|
||||
pnpm -r typecheck # clean
|
||||
pnpm -r test # core: 234/234 pass (jsdom env: 230 pass, 4 skipped)
|
||||
# cli: 157/158 pass — 1 flaky failure = finding M-1
|
||||
```
|
||||
Reference in new issue
Block a user