security: fix critical findings from the second audit

Four independent reviewers went over the branch. The worst findings were in
code I had just written and reviewed myself, which is the argument for
having done this.

CRITICAL

- A mnemonic was validated in one form and derived from in another. @scure's
  normalize only counts words, so a phrase with a trailing newline passed
  validation and then derived a completely different seed, while Python
  canonicalizes and derives the correct one. The result was a vault encrypted
  under a key that neither implementation -- nor the user's written-down
  phrase -- could ever reproduce. Mnemonics are now canonicalized (NFKD,
  lowercase, single-space) once, and every derivation site uses that form:
  Bip85.fromMnemonic, deriveIndexKeyBytes, deriveTotpSecret, deriveKeyIndex
  and generateFingerprint.

- atomicWrite ignored short writes. `handle.write()` may write fewer bytes
  than asked (ENOSPC, RLIMIT_FSIZE, EINTR); the truncated temp file was then
  fsynced and renamed over the only copy of the vault, with exit code 0 and
  no stderr -- inside the function whose comment promised crash safety. It
  now loops until every byte lands, fails loudly otherwise, and removes the
  temp file on any failure.

- `fingerprint add` overwrote an existing profile. It consulted only
  fingerprints.json, which readFingerprints returns empty for whenever the
  file is corrupt or unreadable, then rewrote parent_seed.enc and the index.
  Restoring a profile directory from backup and re-registering it -- the
  canonical recovery gesture -- destroyed the vault. It now refuses if the
  seed or index file already exists.

- Entry-id allocation could collide. A non-numeric or above-2^53 key (which
  can arrive through import or a merge) made nextIndex return NaN or stop
  incrementing, and insert overwrote blindly, so each new entry destroyed the
  previous one. Both are now rejected, and insert refuses to overwrite.

HIGH

- RelayPool accepted any event a relay chose to send: verifyEvent only proves
  an event is internally consistent, never that it came from the pubkey,
  kind, or id we asked for. Events are now structurally validated (including
  created_at being a finite safe integer -- "1e999" parsed to Infinity and
  sorted ahead of everything forever) and matched against the filter.

- Snapshot handling had no resource budget: a decompression bomb turned 20 KB
  of relay data into 20 MB, unbounded. Chunk count, chunk size, total
  compressed and total decompressed bytes are now capped, the gunzip is
  aborted mid-stream when it exceeds the budget, and each chunk must match
  the size its signed manifest declares.

- Manifest tie-breaking used relay arrival order, letting a relay pick which
  of two same-second snapshots won; it now breaks ties on event id, which no
  party can steer. Falling back to an older manifest is recorded so callers
  can surface a downgrade.

- Argon2/PBKDF2 parameters were read unbounded from the kdf block, which sits
  outside the AEAD and is writable by anyone with directory access: 1e9 time
  cost hung indefinitely, and this core targets browsers too. Now clamped.

- Config and fingerprint-registry writes held no lock at all, losing 4 of 6
  concurrent updates in testing. Both now use a read-modify-write lock, and
  the vault lock itself only breaks a lock whose owner pid is gone, steals it
  atomically by rename, and releases only if it still owns it.

- Plaintext egress (document export, vault export) used writeFile's mode,
  which applies only on creation, and followed symlinks -- an existing
  world-readable file or a planted symlink received the secret. Both now go
  through atomicWrite, which creates a fresh 0600 inode.

MEDIUM

- canonicalJson did not match Python. Exponent padding (1e-7 vs 1e-07), the
  1e16 threshold, -0, and key ordering (UTF-16 code units vs code points) all
  differed, and the code comment claiming they agreed was false. Since entry
  hashes decide merge winners, two clients could keep different versions of
  an entry forever. Now matched, with 24 differential fixtures generated from
  CPython, and values above 2^53 refused rather than guessed at.

- "__proto__" as an entry or tombstone key silently vanished and polluted the
  prototype; containers are now null-prototype. Python truthiness differs
  from JS on empty containers, so `"_deleted": []` tombstoned an entry in JS
  and kept it in Python; now matched.

- Policy blocks with JSON null produced the literal string "null" as the
  special-character alphabet. TOTP secrets that pyotp accepts (lowercase,
  unpadded) were rejected. Both fixed.

- hexToBytes accepted malformed input ("1z" -> 0x01, "-1" -> 0xff) where
  Python raises; relay-supplied ids and signatures reach it.

- Password/fingerprint normalization used JS trim(), which strips a different
  set than Python's strip(), changing derived keys; now uses Python's set.

Also removes a scratch file a reviewer's session left committed.

220 core + 71 CLI tests green; 36/36 cross-implementation checks green.

Co-Authored-By: Claude Fable 5 <[email protected]>

js/packages/cli/src/vaultFile.ts

@@ -3,7 +3,7 @@
  * or without the JSON kdf/ct wrapper) unlocked by the parent seed mnemonic.
  */
 
-import { readFile, open, rename, rm, stat, chmod } from "node:fs/promises";
+import { readFile, open, rename, rm, stat } from "node:fs/promises";
 import { basename, dirname, join } from "node:path";
 import { randomBytes } from "node:crypto";
 import process from "node:process";
@@ -81,16 +81,39 @@ export async function atomicWrite(path: string, data: Uint8Array): Promise<void>
   const dir = dirname(path);
   const tmp = join(dir, `.${basename(path)}.${process.pid}.${randomBytes(6).toString("hex")}.tmp`);
   const handle = await open(tmp, "wx", 0o600);
+  let renamed = false;
   try {
-    await handle.write(data);
+    // A short write is not an error in POSIX, and ignoring it is how a
+    // truncated file gets fsynced and renamed over the only good copy —
+    // silently, with a success exit code. Loop until everything lands and
+    // fail loudly if it cannot.
+    let written = 0;
+    while (written < data.length) {
+      const { bytesWritten } = await handle.write(data, written, data.length - written);
+      if (bytesWritten <= 0) {
+        throw new Error(
+          `short write to ${tmp}: wrote ${written} of ${data.length} bytes`,
+        );
+      }
+      written += bytesWritten;
+    }
+    if (written !== data.length) {
+      throw new Error(`short write to ${tmp}: wrote ${written} of ${data.length} bytes`);
+    }
+    // fchmod on the handle, not the path: a path-based chmod after the file
+    // is visible can be redirected through a symlink.
+    await handle.chmod(0o600);
     await handle.sync();
-  } finally {
     await handle.close();
+    await rename(tmp, path);
+    renamed = true;
+  } finally {
+    if (!renamed) {
+      // Never leave a partial ciphertext copy behind.
+      await handle.close().catch(() => {});
+      await rm(tmp, { force: true }).catch(() => {});
+    }
   }
-  await rename(tmp, path);
-  // Repair permissions if the target already existed with looser modes:
-  // writeFile's mode argument applies only on creation.
-  await chmod(path, 0o600);
   const dirHandle = await open(dir, "r");
   try {
     await dirHandle.sync();
@@ -110,38 +133,84 @@ export async function atomicWrite(path: string, data: Uint8Array): Promise<void>
  */
 const LOCK_STALE_MS = 30_000;
 
+/** Is a pid still running (and therefore still holding its lock)? */
+function pidAlive(pid: number): boolean {
+  if (!Number.isInteger(pid) || pid <= 0) return false;
+  try {
+    process.kill(pid, 0);
+    return true;
+  } catch (e) {
+    // EPERM means it exists but belongs to someone else — still alive.
+    return (e as NodeJS.ErrnoException).code === "EPERM";
+  }
+}
+
 export async function withVaultLock<T>(path: string, fn: () => Promise<T>): Promise<T> {
   const lockPath = `${path}.lock`;
+  // A token unique to this acquisition. Releasing or breaking a lock without
+  // checking it is how two processes end up both believing they hold it:
+  // rm-then-create is not atomic, so a slow holder's lock can be deleted by
+  // a racer that already decided it was stale.
+  const token = `${process.pid}:${randomBytes(12).toString("hex")}`;
   const deadline = Date.now() + LOCK_STALE_MS;
+
   for (;;) {
     try {
       const handle = await open(lockPath, "wx", 0o600);
-      await handle.write(String(process.pid));
-      await handle.close();
+      try {
+        await handle.write(token);
+        await handle.sync();
+      } finally {
+        await handle.close();
+      }
       break;
     } catch (e) {
       if ((e as NodeJS.ErrnoException).code !== "EEXIST") throw e;
+
+      let holder = "";
       let age = 0;
       try {
+        holder = await readFile(lockPath, "utf8");
         age = Date.now() - (await stat(lockPath)).mtimeMs;
       } catch {
-        continue; // holder released it between our attempts
+        continue; // released between our attempts
       }
-      if (age > LOCK_STALE_MS) {
-        await rm(lockPath, { force: true });
+      const holderPid = Number.parseInt(holder.split(":")[0] ?? "", 10);
+
+      // Only break a lock whose owner is demonstrably gone. Age alone is a
+      // guess — a slow disk or a suspended laptop is not a crash.
+      if (!pidAlive(holderPid) && age > LOCK_STALE_MS) {
+        // Steal atomically: move the stale file aside and only proceed if
+        // this process is the one that managed to move it.
+        const claim = `${lockPath}.claim.${randomBytes(8).toString("hex")}`;
+        try {
+          await rename(lockPath, claim);
+          await rm(claim, { force: true });
+        } catch {
+          // Another process won the steal; fall through and retry.
+        }
         continue;
       }
       if (Date.now() > deadline) {
         throw new Error(
-          `another SeedPass process is holding ${lockPath}; retry, or remove it if no process is running`,
+          `another SeedPass process (pid ${holderPid || "unknown"}) is holding ` +
+            `${lockPath}; retry, or remove it if no such process is running`,
         );
       }
       await new Promise((r) => setTimeout(r, 25));
     }
   }
+
   try {
     return await fn();
   } finally {
-    await rm(lockPath, { force: true });
+    // Release only if we still own it: a lock broken out from under us
+    // belongs to someone else now, and deleting it would evict them.
+    try {
+      const current = await readFile(lockPath, "utf8");
+      if (current === token) await rm(lockPath, { force: true });
+    } catch {
+      // Already gone.
+    }
   }
 }