From ee330c52f77713715e3b134dd0380f6720148189 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:42:58 -0400 Subject: [PATCH] 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 --- js/packages/cli/src/appDir.ts | 33 ++- js/packages/cli/src/configFile.ts | 34 ++- js/packages/cli/src/program.ts | 61 ++++-- js/packages/cli/src/vaultFile.ts | 97 +++++++-- js/packages/core/.audit1.mts | 9 - js/packages/core/src/derive/bip85.ts | 19 +- js/packages/core/src/derive/fingerprint.ts | 5 +- js/packages/core/src/derive/password.ts | 8 +- js/packages/core/src/derive/totp.ts | 20 +- js/packages/core/src/index.ts | 2 + js/packages/core/src/sync/canonical.ts | 70 ++++++- js/packages/core/src/sync/events.ts | 55 +++++ js/packages/core/src/sync/merge.ts | 34 ++- js/packages/core/src/sync/relay.ts | 15 +- js/packages/core/src/sync/snapshot.ts | 73 +++++-- js/packages/core/src/sync/syncFlows.ts | 42 +++- js/packages/core/src/util/bytes.ts | 25 ++- js/packages/core/src/vault/entryOps.ts | 38 +++- js/packages/core/src/vault/indexKey.ts | 5 +- js/packages/core/src/vault/passwordKdf.ts | 43 +++- js/packages/core/test/canonical.test.ts | 43 ++++ .../test-vectors/fixtures/canonical_json.json | 198 ++++++++++++++++++ js/packages/test-vectors/src/index.ts | 11 + scripts/canonical_json_cases.py | 102 +++++++++ 24 files changed, 936 insertions(+), 106 deletions(-) delete mode 100644 js/packages/core/.audit1.mts create mode 100644 js/packages/core/test/canonical.test.ts create mode 100644 js/packages/test-vectors/fixtures/canonical_json.json create mode 100644 scripts/canonical_json_cases.py diff --git a/js/packages/cli/src/appDir.ts b/js/packages/cli/src/appDir.ts index 7afe753e..7e7007b8 100644 --- a/js/packages/cli/src/appDir.ts +++ b/js/packages/cli/src/appDir.ts @@ -12,7 +12,7 @@ import { mkdir, readFile, rm } from "node:fs/promises"; import { existsSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join, resolve } from "node:path"; -import { atomicWrite } from "./vaultFile.js"; +import { atomicWrite, withVaultLock } from "./vaultFile.js"; import process from "node:process"; import { base64url } from "@scure/base"; import { @@ -105,7 +105,22 @@ export class AppDir { async writeFingerprints(data: FingerprintsFile): Promise { await mkdir(this.root, { recursive: true }); // indent=4 matches the Python writer - await atomicWrite(this.fingerprintsPath, utf8(JSON.stringify(data, null, 4))); + await withVaultLock(this.fingerprintsPath, () => + atomicWrite(this.fingerprintsPath, utf8(JSON.stringify(data, null, 4))), + ); + } + + /** Read-modify-write the registry under one lock. */ + async mutateFingerprints( + fn: (data: FingerprintsFile) => T | Promise, + ): Promise { + await mkdir(this.root, { recursive: true }); + return withVaultLock(this.fingerprintsPath, async () => { + const data = await this.readFingerprints(); + const result = await fn(data); + await atomicWrite(this.fingerprintsPath, utf8(JSON.stringify(data, null, 4))); + return result; + }); } /** Create a profile from a mnemonic; returns its fingerprint. */ @@ -121,6 +136,20 @@ export class AppDir { throw new Error(`profile ${fingerprint} already exists`); } const dir = this.profileDir(fingerprint); + // The registry is not proof of absence: readFingerprints swallows a + // corrupt or unreadable fingerprints.json and reports no profiles. The + // files on disk are what matter — restoring a profile directory from a + // backup and re-registering it is the canonical "recover my vault" + // gesture, and it must not overwrite the seed and index. + for (const existing of [PARENT_SEED_FILENAME, INDEX_FILENAME]) { + if (existsSync(join(dir, existing))) { + throw new Error( + `refusing to create profile ${fingerprint}: ${join(dir, existing)} already ` + + `exists. If this profile is missing from fingerprints.json, repair the ` + + `registry rather than re-adding the profile.`, + ); + } + } await mkdir(dir, { recursive: true, mode: 0o700 }); // parent_seed.enc: kdf/ct wrapper around a V3 blob under the password key. diff --git a/js/packages/cli/src/configFile.ts b/js/packages/cli/src/configFile.ts index 3bc1791f..f592d50a 100644 --- a/js/packages/cli/src/configFile.ts +++ b/js/packages/cli/src/configFile.ts @@ -14,7 +14,7 @@ import { utf8, } from "@seedpass/core"; import { CONFIG_FILENAME } from "./appDir.js"; -import { atomicWrite } from "./vaultFile.js"; +import { atomicWrite, withVaultLock } from "./vaultFile.js"; export const DEFAULT_RELAYS = [ "wss://relay.snort.social", @@ -68,7 +68,33 @@ export async function saveConfig( mnemonic: string, config: Record, ): Promise { - const key = deriveIndexKeyBytes(mnemonic); - const payload = await encryptV3(key, utf8(JSON.stringify(config))); - await atomicWrite(join(profileDir, CONFIG_FILENAME), payload); + const path = join(profileDir, CONFIG_FILENAME); + await withVaultLock(path, async () => { + const key = deriveIndexKeyBytes(mnemonic); + const payload = await encryptV3(key, utf8(JSON.stringify(config))); + await atomicWrite(path, payload); + }); +} + +/** + * Read, modify and write the config under one lock. + * + * `config set`, `nostr add-relay` and `nostr remove-relay` are all + * read-modify-write cycles. Locking only the write loses concurrent updates + * — the config holds the relay list and password/pin hashes, so a lost + * update there is not cosmetic. + */ +export async function mutateConfig( + profileDir: string, + mnemonic: string, + fn: (config: Record) => T | Promise, +): Promise { + const path = join(profileDir, CONFIG_FILENAME); + return withVaultLock(path, async () => { + const config = await loadConfig(profileDir, mnemonic); + const result = await fn(config); + const key = deriveIndexKeyBytes(mnemonic); + await atomicWrite(path, await encryptV3(key, utf8(JSON.stringify(config)))); + return result; + }); } diff --git a/js/packages/cli/src/program.ts b/js/packages/cli/src/program.ts index e6348dad..f8ea25b4 100644 --- a/js/packages/cli/src/program.ts +++ b/js/packages/cli/src/program.ts @@ -63,6 +63,7 @@ import { deriveSshKeyPair, derivePgpKey, sshPublicKeyOpenSsh, + utf8, } from "@seedpass/core"; import { readFile, writeFile, mkdir } from "node:fs/promises"; import { existsSync, statSync } from "node:fs"; @@ -130,7 +131,7 @@ function resolveHome(p: string): string { return p.startsWith("~") ? join(homedir(), p.slice(1)) : p; } import { AppDir, resolveAppDir, INDEX_FILENAME } from "./appDir.js"; -import { loadConfig, saveConfig } from "./configFile.js"; +import { loadConfig, saveConfig, mutateConfig } from "./configFile.js"; import { AgentClient, AgentDaemon, agentSocketPath, DEFAULT_TTL_SECONDS } from "./agent.js"; import { AuditLog } from "./audit.js"; @@ -647,7 +648,10 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { if (existsSync(dest) && !o.overwrite) { throw new Error(`File already exists: ${dest} (use --overwrite)`); } - await writeFile(dest, hit.entry.content, { mode: 0o600 }); + // atomicWrite creates a fresh 0600 inode and renames it into place, so + // a pre-existing world-readable file (or a symlink pointing somewhere + // else) cannot end up holding this plaintext. + await atomicWrite(dest, utf8(hit.entry.content)); io.out(JSON.stringify({ exported: dest, ref: hit.ref, bytes: hit.entry.content.length })); }); @@ -963,15 +967,15 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { const app = new AppDir(resolveAppDir(opts.appDir)); const fp = await currentFingerprint(app, opts); const mnemonic = await resolveMnemonic(app, opts); - const cfg = await loadConfig(app.profileDir(fp), mnemonic); let parsed: unknown = value; try { parsed = JSON.parse(value); } catch { // keep as string } - cfg[key] = parsed; - await saveConfig(app.profileDir(fp), mnemonic, cfg); + await mutateConfig(app.profileDir(fp), mnemonic, (cfg) => { + cfg[key] = parsed; + }); io.out(JSON.stringify({ [key]: parsed })); }); @@ -1008,10 +1012,13 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { .description("add a relay URL") .action(async (url: string) => { if (!/^wss?:\/\//.test(url)) throw new Error("relay URL must start with ws:// or wss://"); - const { app, fp, mnemonic, cfg } = await nostrContext(program.opts()); - const relays = cfg["relays"] as string[]; - if (!relays.includes(url)) relays.push(url); - await saveConfig(app.profileDir(fp), mnemonic, cfg); + const { app, fp, mnemonic } = await nostrContext(program.opts()); + const relays = await mutateConfig(app.profileDir(fp), mnemonic, (cfg) => { + const list = Array.isArray(cfg["relays"]) ? (cfg["relays"] as string[]) : []; + if (!list.includes(url)) list.push(url); + cfg["relays"] = list; + return list; + }); io.out(JSON.stringify({ relays })); }); @@ -1019,15 +1026,16 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { .command("remove-relay ") .description("remove a relay by 1-based index") .action(async (indexStr: string) => { - const { app, fp, mnemonic, cfg } = await nostrContext(program.opts()); - const relays = cfg["relays"] as string[]; + const { app, fp, mnemonic } = await nostrContext(program.opts()); const i = parseIntOption(indexStr, "index", { min: 1 }) - 1; - if (!Number.isInteger(i) || i < 0 || i >= relays.length) { - throw new Error(`index out of range 1..${relays.length}`); - } - if (relays.length === 1) throw new Error("at least one relay must remain"); - relays.splice(i, 1); - await saveConfig(app.profileDir(fp), mnemonic, cfg); + const relays = await mutateConfig(app.profileDir(fp), mnemonic, (cfg) => { + const list = Array.isArray(cfg["relays"]) ? (cfg["relays"] as string[]) : []; + if (i >= list.length) throw new Error(`index out of range 1..${list.length}`); + if (list.length === 1) throw new Error("at least one relay must remain"); + list.splice(i, 1); + cfg["relays"] = list; + return list; + }); io.out(JSON.stringify({ relays })); }); @@ -1106,7 +1114,12 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { await decryptPayload(indexKey, parseEncryptedFile(delta).ciphertext), ), ); - remote = mergeIndexPayloads(remote, incoming, sha256Hex(delta).slice(0, 16)); + // preserve-current here too: the default would refuse any + // Python profile that carries atlas state as soon as one delta + // exists. + remote = mergeIndexPayloads(remote, incoming, sha256Hex(delta).slice(0, 16), { + index0: "preserve-current", + }); deltaCount++; } } @@ -1343,18 +1356,22 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { .command("export ") .description("write a portable backup (encrypted by default)") .option("--plaintext", "HIGH RISK: export without encryption") - .action(async (destFile: string, cmdOpts: { plaintext?: boolean }) => { + .option("--overwrite", "replace an existing file") + .action(async (destFile: string, o: { plaintext?: boolean; overwrite?: boolean }) => { const vault = await openFromOptions(program.opts()); const wrapper = await exportBackup(vault.index as Record, { mnemonic: vault.mnemonic, fingerprint: generateFingerprint(vault.mnemonic), - encrypt: !cmdOpts.plaintext, + encrypt: !o.plaintext, }); - await writeFile(destFile, JSON.stringify(wrapper, null, 2) + "\n", { mode: 0o600 }); + if (existsSync(destFile) && !o.overwrite) { + throw new Error(`File already exists: ${destFile} (use --overwrite)`); + } + await atomicWrite(destFile, utf8(JSON.stringify(wrapper, null, 2) + "\n")); io.out( JSON.stringify({ exported: destFile, - encrypted: !cmdOpts.plaintext, + encrypted: !o.plaintext, checksum: wrapper.checksum, }), ); diff --git a/js/packages/cli/src/vaultFile.ts b/js/packages/cli/src/vaultFile.ts index b2175be1..eaa92d5b 100644 --- a/js/packages/cli/src/vaultFile.ts +++ b/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 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 */ 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(path: string, fn: () => Promise): Promise { 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. + } } } diff --git a/js/packages/core/.audit1.mts b/js/packages/core/.audit1.mts deleted file mode 100644 index 2fb52245..00000000 --- a/js/packages/core/.audit1.mts +++ /dev/null @@ -1,9 +0,0 @@ -import { hexToBytes, bytesToHex } from "./src/util/bytes.js"; -const cases = ["1z", "-1", "+1", " 1", "0x", "1 ", "gg", "1\n", "aA", "1e", "Infinity".slice(0,2), "e5", "1_", ".5", "0b"]; -for (const c of cases) { - try { console.log(JSON.stringify(c), "->", bytesToHex(hexToBytes(c))); } - catch (e) { console.log(JSON.stringify(c), "-> THROW", (e as Error).message); } -} -// bech32 roundtrip malleability -import { hexToBech32 } from "./src/derive/nostr.js"; -console.log("npub from 'zz'*32:", (()=>{try{return hexToBech32("zz".repeat(32) as string, "npub")}catch(e){return "throw "+(e as Error).message}})()); diff --git a/js/packages/core/src/derive/bip85.ts b/js/packages/core/src/derive/bip85.ts index ff9880bb..1bb72847 100644 --- a/js/packages/core/src/derive/bip85.ts +++ b/js/packages/core/src/derive/bip85.ts @@ -19,9 +19,24 @@ import { import { wordlist as english } from "@scure/bip39/wordlists/english.js"; import { utf8 } from "../util/bytes.js"; +/** + * Canonical form of a mnemonic: NFKD, lowercase, single-space separated. + * + * This must be applied before BOTH validation and derivation. `@scure`'s + * normalize only counts words and then PBKDF2s the raw string, so a phrase + * with a trailing newline validates yet derives a completely different seed + * — while Python's Bip39SeedGenerator canonicalizes and derives the correct + * one. Validating one string and deriving from another produced a vault + * that neither implementation, nor the user's written-down phrase, could + * ever reopen. + */ +export function canonicalizeMnemonic(mnemonic: string): string { + return mnemonic.normalize("NFKD").trim().toLowerCase().split(/\s+/u).join(" "); +} + /** True when `mnemonic` is a valid English BIP-39 phrase (checksum included). */ export function isValidMnemonic(mnemonic: string): boolean { - return scureValidateMnemonic(mnemonic.trim(), english); + return scureValidateMnemonic(canonicalizeMnemonic(mnemonic), english); } /** @@ -60,7 +75,7 @@ export class Bip85 { } static fromMnemonic(mnemonic: string, passphrase = ""): Bip85 { - return new Bip85(mnemonicToSeedSync(mnemonic, passphrase)); + return new Bip85(mnemonicToSeedSync(canonicalizeMnemonic(mnemonic), passphrase)); } deriveEntropy(opts: DeriveEntropyOptions): Uint8Array { diff --git a/js/packages/core/src/derive/fingerprint.ts b/js/packages/core/src/derive/fingerprint.ts index 5ff676e2..5479f246 100644 --- a/js/packages/core/src/derive/fingerprint.ts +++ b/js/packages/core/src/derive/fingerprint.ts @@ -5,9 +5,12 @@ import { sha256 } from "@noble/hashes/sha2.js"; import { bytesToHex, utf8 } from "../util/bytes.js"; +import { canonicalizeMnemonic } from "./bip85.js"; export function generateFingerprint(seedPhrase: string, length = 16): string { - const normalized = seedPhrase.trim().toLowerCase(); + // Same canonical form as derivation, so the fingerprint identifies the + // vault the seed actually opens. + const normalized = canonicalizeMnemonic(seedPhrase); const digest = sha256(utf8(normalized)); return bytesToHex(digest).slice(0, length).toUpperCase(); } diff --git a/js/packages/core/src/derive/password.ts b/js/packages/core/src/derive/password.ts index 2cdbb902..31041b1a 100644 --- a/js/packages/core/src/derive/password.ts +++ b/js/packages/core/src/derive/password.ts @@ -371,7 +371,13 @@ function generateV2( */ export function passwordPolicyFromRecord(raw: unknown): PasswordPolicy { if (typeof raw !== "object" || raw === null || Array.isArray(raw)) return {}; - const o = raw as Record; + const source = raw as Record; + // A JSON null means "not set". String(null) is "null", which would turn + // the special-character alphabet into the letters n, u, l. + const o: Record = {}; + for (const [k, v] of Object.entries(source)) { + if (v !== null) o[k] = v; + } const num = (v: unknown): number | undefined => v === undefined || v === null ? undefined : Math.trunc(Number(v)); return { diff --git a/js/packages/core/src/derive/totp.ts b/js/packages/core/src/derive/totp.ts index 28079acc..88f9fe49 100644 --- a/js/packages/core/src/derive/totp.ts +++ b/js/packages/core/src/derive/totp.ts @@ -13,13 +13,15 @@ import { base32 } from "@scure/base"; import { mnemonicToSeedSync } from "@scure/bip39"; import { HDKey } from "@scure/bip32"; import { utf8 } from "../util/bytes.js"; +import { canonicalizeMnemonic } from "./bip85.js"; const TOTP_PURPOSE = 39; const TOTP_INT = 0x544f5450; // 1414485072 = int.from_bytes(b"TOTP", "big") /** Derive the base32 TOTP secret for an index from a mnemonic or raw seed. */ export function deriveTotpSecret(seed: string | Uint8Array, index: number): string { - const seedBytes = typeof seed === "string" ? mnemonicToSeedSync(seed) : seed; + const seedBytes = + typeof seed === "string" ? mnemonicToSeedSync(canonicalizeMnemonic(seed)) : seed; const root = HDKey.fromMasterSeed(seedBytes); const path = `m/83696968'/${TOTP_PURPOSE}'/${TOTP_INT}'/${index}'`; const child = root.derive(path); @@ -29,9 +31,23 @@ export function deriveTotpSecret(seed: string | Uint8Array, index: number): stri return base32.encode(hashed.slice(0, 20)); } +/** + * Accept the base32 forms pyotp does. + * + * pyotp calls b32decode(casefold=True) and pads, so lowercase and + * unpadded secrets work there. @scure's decoder rejects both, which would + * make imported entries — including SeedPass's own, since random_totp_secret + * strips padding — unusable here but fine in Python. + */ +function decodeTotpSecret(secretB32: string): Uint8Array { + const cleaned = secretB32.trim().replace(/\s+/g, "").toUpperCase().replace(/=+$/, ""); + const padded = cleaned.padEnd(Math.ceil(cleaned.length / 8) * 8, "="); + return base32.decode(padded); +} + /** RFC 4226 HOTP with SHA-1 (the TOTP default the Python side uses via pyotp). */ export function hotp(secretB32: string, counter: number, digits = 6): string { - const key = base32.decode(secretB32); + const key = decodeTotpSecret(secretB32); const msg = new Uint8Array(8); const view = new DataView(msg.buffer); view.setBigUint64(0, BigInt(counter), false); diff --git a/js/packages/core/src/index.ts b/js/packages/core/src/index.ts index 75df372e..64cc637c 100644 --- a/js/packages/core/src/index.ts +++ b/js/packages/core/src/index.ts @@ -9,6 +9,7 @@ export { Bip85, isValidMnemonic, assertValidMnemonic, + canonicalizeMnemonic, type DeriveEntropyOptions, } from "./derive/bip85.js"; export { @@ -50,6 +51,7 @@ export { deriveKeyFromPassword, deriveKeyFromPasswordArgon2, kdfConfigSchema, + KDF_LIMITS, type KdfConfig, } from "./vault/passwordKdf.js"; export { fernetDecrypt, FernetError } from "./vault/fernet.js"; diff --git a/js/packages/core/src/sync/canonical.ts b/js/packages/core/src/sync/canonical.ts index b80a5072..624fcc1e 100644 --- a/js/packages/core/src/sync/canonical.ts +++ b/js/packages/core/src/sync/canonical.ts @@ -41,13 +41,68 @@ function encodeString(s: string): string { return out + '"'; } +/** + * Numbers, formatted the way CPython's json module would. + * + * JS and Python disagree in several places, and every disagreement changes + * an entry hash — which decides merge winners, so a mismatch means two + * clients can diverge permanently. Handled here: + * + * value JS toString CPython json + * 1e16 10000000000000000 1e+16 + * 1e-7 1e-7 1e-07 + * 1e23 1e+23 1e+23 (agrees) + * -0 0 -0.0 + * + * Integral values are emitted without a decimal point, matching Python ints; + * SeedPass data carries integers (timestamps, lengths, counts) and never + * float-typed whole numbers, so the int/float ambiguity CPython would + * otherwise expose does not arise. A non-integral or non-safe value would + * be ambiguous, so it is rejected rather than guessed at. + */ function encodeNumber(n: number): string { if (!Number.isFinite(n)) throw new Error("non-finite number in canonical JSON"); - if (Number.isInteger(n) && Object.is(n, -0) === false) return n.toString(); - // Python repr(float) and JS toString agree on shortest-roundtrip decimals - // for doubles; integral floats (1.0) cannot be distinguished from ints in - // JSON-parsed data, so both sides serialize them identically. - return n.toString(); + if (Object.is(n, -0)) return "-0.0"; + if (Number.isInteger(n)) { + if (!Number.isSafeInteger(n)) { + throw new Error( + `integer ${n} exceeds the safe range; its canonical form would differ ` + + `from the Python reference and break hash agreement`, + ); + } + return n.toString(); + } + return pythonFloatRepr(n); +} + +/** CPython repr() for a float: shortest round-trip, two-digit exponent. */ +function pythonFloatRepr(n: number): string { + const s = n.toString(); + const m = /^(-?)(\d(?:\.\d+)?)e([+-])(\d+)$/.exec(s); + if (m) { + // JS writes 1e-7; CPython writes 1e-07 (exponent padded to two digits). + const [, sign, mantissa, expSign, expDigits] = m; + return `${sign}${mantissa}e${expSign}${expDigits!.padStart(2, "0")}`; + } + if (Math.abs(n) >= 1e16) { + // CPython switches to exponent form at 1e16; JS does so only at 1e21. + const exp = n.toExponential(); + const em = /^(-?\d(?:\.\d+)?)e([+-])(\d+)$/.exec(exp)!; + return `${em[1]}e${em[2]}${em[3]!.padStart(2, "0")}`; + } + return s; +} + +/** Code-point ordering, matching Python's sorted() on str. */ +function compareCodePoints(a: string, b: string): number { + const ai = Array.from(a); + const bi = Array.from(b); + const len = Math.min(ai.length, bi.length); + for (let i = 0; i < len; i++) { + const d = ai[i]!.codePointAt(0)! - bi[i]!.codePointAt(0)!; + if (d !== 0) return d; + } + return ai.length - bi.length; } export function canonicalJson(value: unknown): string { @@ -67,7 +122,10 @@ export function canonicalJson(value: unknown): string { if (proto !== Object.prototype && proto !== null) { throw new Error("non-plain object in canonical JSON"); } - const keys = Object.keys(value as Record).sort(); + // Python sorts by code point; JS's default sort compares UTF-16 code + // units, which disagree once a supplementary-plane key (an emoji in a + // tag, say) sits alongside one at or above U+E000. + const keys = Object.keys(value as Record).sort(compareCodePoints); const parts = keys.map( (k) => encodeString(k) + ":" + canonicalJson((value as Record)[k]), ); diff --git a/js/packages/core/src/sync/events.ts b/js/packages/core/src/sync/events.ts index e1a6970b..b6fb0d1b 100644 --- a/js/packages/core/src/sync/events.ts +++ b/js/packages/core/src/sync/events.ts @@ -91,7 +91,62 @@ export function signEvent(privateKeyHex: string, unsigned: UnsignedEvent): Nostr } /** Verify an event's id derivation and BIP-340 signature. */ +/** + * Structural validation of an event received from a relay. + * + * The wire is untrusted: `parseRelayMessage` casts whatever JSON arrived, so + * types must be checked before anything reads them. `created_at` in + * particular is interpolated into the id preimage and used for ordering — + * `1e999` parses to Infinity and would sort ahead of every real event + * forever, and a string collapses comparisons to NaN. + */ +export function isWellFormedEvent(event: unknown): event is NostrEvent { + if (typeof event !== "object" || event === null) return false; + const e = event as Record; + if (typeof e["id"] !== "string" || !/^[0-9a-f]{64}$/.test(e["id"])) return false; + if (typeof e["pubkey"] !== "string" || !/^[0-9a-f]{64}$/.test(e["pubkey"])) return false; + if (typeof e["sig"] !== "string" || !/^[0-9a-f]{128}$/.test(e["sig"])) return false; + if (typeof e["content"] !== "string") return false; + if ( + typeof e["created_at"] !== "number" || + !Number.isSafeInteger(e["created_at"]) || + e["created_at"] < 0 + ) { + return false; + } + if (typeof e["kind"] !== "number" || !Number.isSafeInteger(e["kind"])) return false; + const tags = e["tags"]; + if (!Array.isArray(tags)) return false; + return tags.every((t) => Array.isArray(t) && t.every((v) => typeof v === "string")); +} + +/** + * Does an event actually satisfy the filter it was requested with? + * + * A relay is asked for a filter, it is not obliged to honour it. Without + * this check, `fetch` accepts an event signed by any key at all — the + * signature only proves internal consistency, not authorship by the pubkey + * we asked for. + */ +export function eventMatchesFilter(event: NostrEvent, filter: Filter): boolean { + if (filter.ids && !filter.ids.includes(event.id)) return false; + if (filter.authors && !filter.authors.includes(event.pubkey)) return false; + if (filter.kinds && !filter.kinds.includes(event.kind)) return false; + if (filter.since !== undefined && event.created_at < filter.since) return false; + if (filter.until !== undefined && event.created_at > filter.until) return false; + for (const tagFilter of ["#d", "#e"] as const) { + const wanted = filter[tagFilter]; + if (wanted) { + const name = tagFilter[1]!; + const values = event.tags.filter((t) => t[0] === name).map((t) => t[1]); + if (!wanted.some((w) => values.includes(w))) return false; + } + } + return true; +} + export function verifyEvent(event: NostrEvent): boolean { + if (!isWellFormedEvent(event)) return false; const expectedId = computeEventId( event.pubkey, event.created_at, diff --git a/js/packages/core/src/sync/merge.ts b/js/packages/core/src/sync/merge.ts index 579c881f..542a8fc0 100644 --- a/js/packages/core/src/sync/merge.ts +++ b/js/packages/core/src/sync/merge.ts @@ -54,8 +54,21 @@ function entryKind(entry: Dict): string { return String(raw).trim().toLowerCase(); } +/** + * Python truthiness, which differs from JS on containers: bool([]) and + * bool({}) are False in Python but Boolean([]) is true in JS. A + * `"_deleted": []` field would tombstone an entry in JS and keep it in + * Python — the two clients would then disagree about whether it exists. + */ +function pyTruthy(value: unknown): boolean { + if (value === null || value === undefined) return false; + if (Array.isArray(value)) return value.length > 0; + if (typeof value === "object") return Object.keys(value as Dict).length > 0; + return Boolean(value); +} + function isDeletedEntry(entry: Dict): boolean { - return Boolean(entry["_deleted"] ?? false) || Boolean(entry["deleted"] ?? false); + return pyTruthy(entry["_deleted"]) || pyTruthy(entry["deleted"]); } function entryHash(entry: Dict): string { @@ -85,7 +98,10 @@ function preferTombstone(current: Dict, incoming: Dict, idx: string): boolean { } function normalizeTombstones(value: unknown): Record { - const out: Record = {}; + // Object.create(null): "__proto__" must behave as an ordinary key. On a + // normal object it silently replaces the prototype instead of being + // stored, which drops the record here and diverges from Python. + const out: Record = Object.create(null); if (!isDict(value)) return out; for (const [k, v] of Object.entries(value)) { if (!isDict(v)) continue; @@ -105,7 +121,7 @@ function mergeTombstones( current: Record, incoming: Record, ): Record { - const merged: Record = { ...current }; + const merged: Record = Object.assign(Object.create(null), current); for (const [idx, rec] of Object.entries(incoming)) { const cur = merged[idx]; if (cur === undefined || preferTombstone(cur, rec, idx)) { @@ -274,7 +290,13 @@ export function mergeIndexPayloads( options, ); - const curEntries: Dict = isDict(out["entries"]) ? (out["entries"] as Dict) : {}; + // Rebuild entries on a null prototype for the same reason: an incoming + // entry keyed "__proto__" would otherwise be dropped and pollute the + // object's prototype chain. + const curEntries: Dict = Object.assign( + Object.create(null) as Dict, + isDict(out["entries"]) ? (out["entries"] as Dict) : {}, + ); const incEntries: Dict = isDict(incoming["entries"]) ? (incoming["entries"] as Dict) : {}; const curMeta: Dict = isDict(out["_sync_meta"]) ? (out["_sync_meta"] as Dict) : {}; const incMeta: Dict = isDict(incoming["_sync_meta"]) ? (incoming["_sync_meta"] as Dict) : {}; @@ -332,7 +354,9 @@ export function mergeIndexPayloads( } // Apply tombstones after the entry merge - for (const [idx, rec] of Object.entries({ ...tombstones })) { + for (const [idx, rec] of Object.entries( + Object.assign(Object.create(null) as Record, tombstones), + )) { const entry = curEntries[idx]; if (!isDict(entry)) continue; const eTs = entryTs(entry); diff --git a/js/packages/core/src/sync/relay.ts b/js/packages/core/src/sync/relay.ts index 649ab689..f14ab95a 100644 --- a/js/packages/core/src/sync/relay.ts +++ b/js/packages/core/src/sync/relay.ts @@ -9,7 +9,9 @@ */ import { + eventMatchesFilter, eventMessage, + isWellFormedEvent, parseRelayMessage, reqMessage, verifyEvent, @@ -138,7 +140,18 @@ export class RelayPool { const parsed = parseRelayMessage(String(raw.data)); if (parsed.type === "EVENT" && parsed.subscriptionId === subId) { const ev = parsed.event; - if (!byId.has(ev.id) && verifyEvent(ev)) byId.set(ev.id, ev); + // A relay is asked for a filter; it is not obliged to obey + // one. Accept an event only if it is well formed, actually + // satisfies a filter we sent (author, kind, id, time, tags) + // and carries a valid signature over its own id. + if ( + !byId.has(ev.id) && + isWellFormedEvent(ev) && + filters.some((f) => eventMatchesFilter(ev, f)) && + verifyEvent(ev) + ) { + byId.set(ev.id, ev); + } } else if ( (parsed.type === "EOSE" || parsed.type === "CLOSED") && parsed.subscriptionId === subId diff --git a/js/packages/core/src/sync/snapshot.ts b/js/packages/core/src/sync/snapshot.ts index 80c88b96..70a7b1f7 100644 --- a/js/packages/core/src/sync/snapshot.ts +++ b/js/packages/core/src/sync/snapshot.ts @@ -18,15 +18,29 @@ import { hkdf } from "@noble/hashes/hkdf.js"; import { mnemonicToSeedSync } from "@scure/bip39"; import { z } from "zod"; import { bytesToHex, concatBytes, utf8 } from "../util/bytes.js"; +import { canonicalizeMnemonic } from "../derive/bip85.js"; export const KIND_MANIFEST = 30070; export const KIND_SNAPSHOT_CHUNK = 30071; export const KIND_DELTA = 30072; +/** + * Resource budgets for relay-supplied snapshots. + * + * A manifest is signed by the user, but a relay chooses WHICH signed + * manifest to serve, and nothing stops a manifest from describing an + * enormous snapshot. These caps bound the work a relay can make a client do + * before the payload is authenticated by decryption. + */ +export const MAX_CHUNKS = 4096; +export const MAX_CHUNK_BYTES = 1_048_576; // 1 MiB +export const MAX_COMPRESSED_BYTES = 64 * 1_048_576; +export const MAX_DECOMPRESSED_BYTES = 256 * 1_048_576; + export const chunkMetaSchema = z .object({ - id: z.string(), - size: z.number().int().nonnegative(), + id: z.string().max(256), + size: z.number().int().nonnegative().max(MAX_CHUNK_BYTES), hash: z.string().regex(/^[0-9a-f]{64}$/), event_id: z.string().nullable().optional().default(null), }) @@ -36,7 +50,7 @@ export const manifestSchema = z .object({ ver: z.number().int(), algo: z.string(), - chunks: z.array(chunkMetaSchema), + chunks: z.array(chunkMetaSchema).max(MAX_CHUNKS), delta_since: z.number().int().nullable().optional().default(null), nonce: z.string().nullable().optional().default(null), index0: z.record(z.string(), z.unknown()).nullable().optional().default(null), @@ -53,17 +67,32 @@ export function parseManifest(json: string): Manifest { async function pipeThrough( data: Uint8Array, stream: { readable: ReadableStream; writable: WritableStream }, + maxOutputBytes = Number.POSITIVE_INFINITY, ): Promise { const writer = stream.writable.getWriter(); const writeDone = writer.write(data as BufferSource).then(() => writer.close()); const reader = stream.readable.getReader(); const parts: Uint8Array[] = []; - for (;;) { - const { done, value } = await reader.read(); - if (done) break; - parts.push(value as Uint8Array); + let total = 0; + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + const chunk = value as Uint8Array; + total += chunk.length; + // Stop a decompression bomb while it is still streaming rather than + // after it has already been materialized. + if (total > maxOutputBytes) { + await reader.cancel(); + throw new Error( + `decompressed output exceeds ${maxOutputBytes} bytes; refusing to continue`, + ); + } + parts.push(chunk); + } + } finally { + await writeDone.catch(() => {}); } - await writeDone; return concatBytes(...parts); } @@ -71,8 +100,11 @@ export async function gzipCompress(data: Uint8Array): Promise { return pipeThrough(data, new CompressionStream("gzip")); } -export async function gzipDecompress(data: Uint8Array): Promise { - return pipeThrough(data, new DecompressionStream("gzip")); +export async function gzipDecompress( + data: Uint8Array, + maxOutputBytes = MAX_DECOMPRESSED_BYTES, +): Promise { + return pipeThrough(data, new DecompressionStream("gzip"), maxOutputBytes); } /** Compress and split the encrypted vault into chunks (prepare_snapshot). */ @@ -120,9 +152,26 @@ export async function reassembleSnapshot( `chunk count mismatch: manifest lists ${manifest.chunks.length}, got ${chunks.length}`, ); } + let compressedTotal = 0; for (let i = 0; i < chunks.length; i++) { const meta = manifest.chunks[i]!; - const actual = bytesToHex(sha256(chunks[i]!)); + const chunk = chunks[i]!; + // The manifest's declared size is signed; a chunk that disagrees with it + // is wrong regardless of what its hash says. + if (chunk.length !== meta.size) { + throw new ChunkVerificationError( + `chunk ${meta.id} size mismatch (manifest says ${meta.size}, got ${chunk.length})`, + meta.id, + ); + } + compressedTotal += chunk.length; + if (compressedTotal > MAX_COMPRESSED_BYTES) { + throw new ChunkVerificationError( + `snapshot exceeds ${MAX_COMPRESSED_BYTES} compressed bytes`, + meta.id, + ); + } + const actual = bytesToHex(sha256(chunk)); if (actual !== meta.hash) { throw new ChunkVerificationError( `chunk ${meta.id} hash mismatch (expected ${meta.hash}, got ${actual})`, @@ -135,7 +184,7 @@ export async function reassembleSnapshot( /** key_index: HKDF chain master -> "seedpass:v1:index" (manager.py KEY_INDEX). */ export function deriveKeyIndex(mnemonic: string): Uint8Array { - const seed = mnemonicToSeedSync(mnemonic); + const seed = mnemonicToSeedSync(canonicalizeMnemonic(mnemonic)); const master = hkdf(sha256, seed, undefined, utf8("seedpass:v1:master"), 32); return hkdf(sha256, master, undefined, utf8("seedpass:v1:index"), 32); } diff --git a/js/packages/core/src/sync/syncFlows.ts b/js/packages/core/src/sync/syncFlows.ts index 2ed37362..c9b21942 100644 --- a/js/packages/core/src/sync/syncFlows.ts +++ b/js/packages/core/src/sync/syncFlows.ts @@ -92,6 +92,12 @@ export interface FetchedSnapshot { manifestEvent: NostrEvent; /** Decompressed, still-encrypted vault payload. */ encrypted: Uint8Array; + /** + * Manifests newer than the one returned that could not be assembled. + * Non-empty means the client was walked backwards — usually because a + * relay withheld a chunk — and the caller should say so out loud. + */ + skippedNewerManifests: string[]; } /** Fetch the newest manifest and all its chunks, verifying every hash. */ @@ -102,20 +108,24 @@ export async function fetchLatestSnapshot( const pubkey = signerPublicKeyHex(privateKeyHex); const manifests = await pool.fetch({ authors: [pubkey], kinds: [KIND_MANIFEST] }); if (manifests.length === 0) return null; - // Newest first. Two snapshots published within the same second tie on - // created_at, so fall back to the order the relay returned them in - // (later = more recently accepted); without this the restore can pick a - // stale manifest after a same-second republish. - const order = new Map(manifests.map((ev, i) => [ev.id, i])); + // Newest first, with a tie-break the relay cannot choose. Arrival order + // is relay-controlled — sorting by it lets a relay decide which of two + // same-second snapshots wins. Event id is a hash of the event's own + // contents, so it gives a stable total order no party can steer. manifests.sort( - (a, b) => b.created_at - a.created_at || (order.get(b.id)! - order.get(a.id)!), + (a, b) => b.created_at - a.created_at || (a.id < b.id ? 1 : a.id > b.id ? -1 : 0), ); + // Falling back to an older manifest is a downgrade: a relay that withholds + // one chunk of the newest snapshot can walk a client backwards through its + // history. Record what was skipped so callers can surface it. + const skipped: string[] = []; for (const manifestEvent of manifests) { let manifest: Manifest; try { manifest = parseManifest(manifestEvent.content); } catch { + skipped.push(manifestEvent.id); continue; } const chunks: Uint8Array[] = []; @@ -125,18 +135,28 @@ export async function fetchLatestSnapshot( ? { ids: [meta.event_id], authors: [pubkey], kinds: [KIND_SNAPSHOT_CHUNK] } : { authors: [pubkey], kinds: [KIND_SNAPSHOT_CHUNK], "#d": [meta.id], limit: 1 }; const events = await pool.fetch(filter); - const match = events.find( - (ev) => bytesToHex(sha256(base64.decode(ev.content))) === meta.hash, - ); + // Decode defensively: base64.decode throws on malformed input, and a + // relay can include any event it likes in the response. An unguarded + // decode here let one extra frame abort the entire restore. + const match = events.find((ev) => { + try { + return bytesToHex(sha256(base64.decode(ev.content))) === meta.hash; + } catch { + return false; + } + }); if (!match) { complete = false; break; } chunks.push(base64.decode(match.content)); } - if (!complete) continue; + if (!complete) { + skipped.push(manifestEvent.id); + continue; + } const encrypted = await reassembleSnapshot(manifest, chunks); - return { manifest, manifestEvent, encrypted }; + return { manifest, manifestEvent, encrypted, skippedNewerManifests: skipped }; } return null; } diff --git a/js/packages/core/src/util/bytes.ts b/js/packages/core/src/util/bytes.ts index 30a024c6..df51b0d0 100644 --- a/js/packages/core/src/util/bytes.ts +++ b/js/packages/core/src/util/bytes.ts @@ -11,6 +11,23 @@ export function hmacSha256Hex(key: Uint8Array, data: Uint8Array): string { return bytesToHex(hmac(sha256, key, data)); } +/** + * Strip the characters Python's str.strip() treats as whitespace. + * + * JS trim() and Python strip() are not the same set: trim() removes U+FEFF + * but leaves U+0085 and U+001C..U+001F, and Python does the reverse. That + * difference changes derived keys and fingerprints between implementations. + */ +const PYTHON_WHITESPACE = "\t\n\v\f\r \u001c\u001d\u001e\u001f\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000"; + +export function pythonStrip(value: string): string { + let start = 0; + let end = value.length; + while (start < end && PYTHON_WHITESPACE.includes(value[start]!)) start++; + while (end > start && PYTHON_WHITESPACE.includes(value[end - 1]!)) end--; + return value.slice(start, end); +} + export function utf8(s: string): Uint8Array { return new TextEncoder().encode(s); } @@ -22,12 +39,14 @@ export function bytesToHex(b: Uint8Array): string { } export function hexToBytes(hex: string): Uint8Array { + // parseInt is prefix-lenient and sign-aware: "1z" parses as 1, "-1" as -1 + // (stored as 0xff). Relay-supplied ids and signatures reach this function, + // so validate the whole string first, as Python's bytes.fromhex does. + if (!/^[0-9a-fA-F]*$/.test(hex)) throw new Error("invalid hex"); if (hex.length % 2 !== 0) throw new Error("odd-length hex"); const out = new Uint8Array(hex.length / 2); for (let i = 0; i < out.length; i++) { - const byte = Number.parseInt(hex.slice(i * 2, i * 2 + 2), 16); - if (Number.isNaN(byte)) throw new Error("invalid hex"); - out[i] = byte; + out[i] = Number.parseInt(hex.slice(i * 2, i * 2 + 2), 16); } return out; } diff --git a/js/packages/core/src/vault/entryOps.ts b/js/packages/core/src/vault/entryOps.ts index f87ccd43..cc57e99b 100644 --- a/js/packages/core/src/vault/entryOps.ts +++ b/js/packages/core/src/vault/entryOps.ts @@ -34,9 +34,34 @@ export function isoFromUnix(ts: number): string { return iso.replace(/\.\d{3}Z$/, "+00:00"); } -/** max(int keys) + 1, or 0 for an empty index (get_next_index). */ +/** + * max(int keys) + 1, or 0 for an empty index (get_next_index). + * + * Every key must be a plain non-negative integer within the safe range. A + * non-numeric key makes Math.max return NaN and a huge key makes +1 a no-op, + * and `insert` would then assign the same id repeatedly — each new entry + * silently overwriting the last. Such an index can arrive through import or + * a merge, so refuse it loudly rather than destroying entries. + */ export function nextIndex(index: VaultIndex): number { - const ids = Object.keys(index.entries).map((k) => parseInt(k, 10)); + const keys = Object.keys(index.entries); + const ids: number[] = []; + for (const key of keys) { + if (!/^(0|[1-9][0-9]*)$/.test(key)) { + throw new Error( + `vault index contains a non-numeric entry id ${JSON.stringify(key)}; ` + + `refusing to allocate a new id that could overwrite an existing entry`, + ); + } + const id = Number(key); + if (!Number.isSafeInteger(id)) { + throw new Error( + `vault index contains entry id ${key}, which is outside the safe integer ` + + `range; refusing to allocate a new id`, + ); + } + ids.push(id); + } return ids.length > 0 ? Math.max(...ids) + 1 : 0; } @@ -54,8 +79,13 @@ function stamp(clock: Clock): { now_unix: number; now_iso: string } { } function insert(index: VaultIndex, id: number, entry: Dict): string { - (index.entries as unknown as Dict)[String(id)] = entry; - return String(id); + const key = String(id); + // Never silently replace an entry: allocation bugs show up here first. + if (Object.prototype.hasOwnProperty.call(index.entries, key)) { + throw new Error(`refusing to overwrite existing entry ${key}`); + } + (index.entries as unknown as Dict)[key] = entry; + return key; } export interface CommonOptions { diff --git a/js/packages/core/src/vault/indexKey.ts b/js/packages/core/src/vault/indexKey.ts index b3a6b914..5f36497a 100644 --- a/js/packages/core/src/vault/indexKey.ts +++ b/js/packages/core/src/vault/indexKey.ts @@ -11,6 +11,7 @@ import { sha256 } from "@noble/hashes/sha2.js"; import { base64url } from "@scure/base"; import { mnemonicToSeedSync } from "@scure/bip39"; import { utf8 } from "../util/bytes.js"; +import { canonicalizeMnemonic } from "../derive/bip85.js"; function kd(root: Uint8Array, info: Uint8Array, length = 32): Uint8Array { return hkdf(sha256, root, undefined, info, length); @@ -18,7 +19,9 @@ function kd(root: Uint8Array, info: Uint8Array, length = 32): Uint8Array { /** Raw 32-byte index encryption key. */ export function deriveIndexKeyBytes(mnemonic: string): Uint8Array { - const seed = mnemonicToSeedSync(mnemonic); + // Canonicalize first: deriving from a raw string that merely passed a + // word-count check yields a key nothing else can reproduce. + const seed = mnemonicToSeedSync(canonicalizeMnemonic(mnemonic)); const master = kd(seed, utf8("seedpass:v1:master")); return kd(master, utf8("seedpass:v1:storage")); } diff --git a/js/packages/core/src/vault/passwordKdf.ts b/js/packages/core/src/vault/passwordKdf.ts index 773e15b8..f467577f 100644 --- a/js/packages/core/src/vault/passwordKdf.ts +++ b/js/packages/core/src/vault/passwordKdf.ts @@ -11,7 +11,7 @@ import { pbkdf2 } from "@noble/hashes/pbkdf2.js"; import { argon2id } from "@noble/hashes/argon2.js"; import { sha256 } from "@noble/hashes/sha2.js"; import { base64, base64url } from "@scure/base"; -import { utf8 } from "../util/bytes.js"; +import { pythonStrip, utf8 } from "../util/bytes.js"; import { z } from "zod"; export const kdfConfigSchema = z @@ -28,7 +28,38 @@ export type KdfConfig = z.infer; function normalizePassword(password: string): Uint8Array { if (!password) throw new Error("Password cannot be empty."); - return utf8(password.normalize("NFKD").trim()); + // Python strips a different whitespace set than JS trim(); using trim() + // here made JS accept passwords Python would reject and derive different + // keys for others. + return utf8(pythonStrip(password.normalize("NFKD"))); +} + +/** + * Bounds for KDF parameters read from a vault file. + * + * The kdf block sits outside the AEAD in the parent-seed wrapper, so anyone + * who can write the profile directory chooses these numbers. Unbounded, they + * are a hang or a multi-gigabyte allocation — and this core also runs in a + * browser tab. + */ +export const KDF_LIMITS = { + maxTimeCost: 10, + maxMemoryCost: 1_048_576, // KiB => 1 GiB + maxParallelism: 16, + maxIterations: 10_000_000, +} as const; + +function boundedParam(value: unknown, fallback: number, max: number, name: string): number { + const n = value === undefined || value === null ? fallback : Number(value); + if (!Number.isFinite(n) || !Number.isInteger(n) || n < 1) { + throw new Error(`KDF parameter ${name} must be a positive integer (got ${String(value)})`); + } + if (n > max) { + throw new Error( + `KDF parameter ${name} is ${n}, above the maximum ${max}; refusing to run it`, + ); + } + return n; } /** @@ -43,7 +74,7 @@ export function deriveKeyFromPassword( const salt = typeof fingerprint === "string" ? sha256(utf8(fingerprint)).slice(0, 16) : fingerprint; const key = pbkdf2(sha256, normalizePassword(password), salt, { - c: iterations, + c: boundedParam(iterations, 100_000, KDF_LIMITS.maxIterations, "iterations"), dkLen: 32, }); return base64url.encode(key); @@ -58,9 +89,9 @@ export function deriveKeyFromPasswordArgon2(password: string, kdf: KdfConfig): s }; const salt = base64.decode(kdf.salt_b64); const key = argon2id(normalizePassword(password), salt, { - t: params.time_cost ?? 2, - m: params.memory_cost ?? 64 * 1024, - p: params.parallelism ?? 8, + t: boundedParam(params.time_cost, 2, KDF_LIMITS.maxTimeCost, "time_cost"), + m: boundedParam(params.memory_cost, 64 * 1024, KDF_LIMITS.maxMemoryCost, "memory_cost"), + p: boundedParam(params.parallelism, 8, KDF_LIMITS.maxParallelism, "parallelism"), dkLen: 32, }); return base64url.encode(key); diff --git a/js/packages/core/test/canonical.test.ts b/js/packages/core/test/canonical.test.ts new file mode 100644 index 00000000..d23bc931 --- /dev/null +++ b/js/packages/core/test/canonical.test.ts @@ -0,0 +1,43 @@ +/** + * canonicalJson must agree with CPython's json.dumps byte-for-byte. + * + * Entry and tombstone hashes are computed over this output and decide merge + * winners, so any disagreement lets two clients keep different versions of + * the same entry and never converge. A security review found real + * divergences here (exponent padding, code-unit vs code-point key order) + * that the older fixtures missed by only ever using ASCII keys and integers. + */ + +import { describe, expect, it } from "vitest"; +import { canonicalJsonCases, canonicalJsonUnsupported } from "@seedpass/test-vectors"; +import { canonicalJson, canonicalHash } from "@seedpass/core"; + +describe("canonical JSON parity with Python", () => { + it.each(canonicalJsonCases.map((c, i) => ({ ...c, i })))( + "case $i matches json.dumps", + ({ value, canonical }) => { + expect(canonicalJson(value)).toBe(canonical); + }, + ); + + it("orders keys by code point, not UTF-16 code unit", () => { + // U+FF01 sorts before U+1F600 by code point; the default JS sort, which + // compares code units, puts the surrogate pair first. + const out = canonicalJson({ "\u{1F600}": 1, "！": 2 }); + expect(out.indexOf("uff01")).toBeLessThan(out.indexOf("ud83d")); + }); + + it("refuses values whose canonical form would be ambiguous", () => { + // Above 2**53 JSON cannot distinguish a Python int from a float, so the + // two implementations cannot be made to agree; refuse rather than guess. + expect(() => canonicalJson({ n: Number.MAX_SAFE_INTEGER + 2 })).toThrow(/safe range/); + expect(() => canonicalJson({ n: Infinity })).toThrow(/non-finite/); + for (const value of canonicalJsonUnsupported) { + expect(() => canonicalJson(value)).toThrow(); + } + }); + + it("hashes are stable for the same logical value", () => { + expect(canonicalHash({ a: 1, b: 2 })).toBe(canonicalHash({ b: 2, a: 1 })); + }); +}); diff --git a/js/packages/test-vectors/fixtures/canonical_json.json b/js/packages/test-vectors/fixtures/canonical_json.json new file mode 100644 index 00000000..5bf27370 --- /dev/null +++ b/js/packages/test-vectors/fixtures/canonical_json.json @@ -0,0 +1,198 @@ +{ + "cases": [ + { + "canonical": "{}", + "value": {} + }, + { + "canonical": "{\"a\":1}", + "value": { + "a": 1 + } + }, + { + "canonical": "{\"a\":1,\"b\":2}", + "value": { + "a": 1, + "b": 2 + } + }, + { + "canonical": "{\"label\":\"site\",\"modified_ts\":1700000000}", + "value": { + "label": "site", + "modified_ts": 1700000000 + } + }, + { + "canonical": "{\"notes\":\"\",\"tags\":[\"b\",\"a\"]}", + "value": { + "notes": "", + "tags": [ + "b", + "a" + ] + } + }, + { + "canonical": "{\"\\uff01\":2,\"\\ud83d\\ude00\":1}", + "value": { + "\uff01": 2, + "\ud83d\ude00": 1 + } + }, + { + "canonical": "{\"z\":1,\"\\u00e9\":2,\"\\u4e2d\":3,\"\\ud83d\\ude00\":4}", + "value": { + "z": 1, + "\u00e9": 2, + "\u4e2d": 3, + "\ud83d\ude00": 4 + } + }, + { + "canonical": "{\"quote\\\"\":\"back\\\\slash\"}", + "value": { + "quote\"": "back\\slash" + } + }, + { + "canonical": "{\"ctrl\":\"\\u0000\\u0001\\u001f\"}", + "value": { + "ctrl": "\u0000\u0001\u001f" + } + }, + { + "canonical": "{\"nl\":\"line\\nbreak\\ttab\"}", + "value": { + "nl": "line\nbreak\ttab" + } + }, + { + "canonical": "{\"unicode\":\"caf\\u00e9 \\u4e2d\\u6587 \\ud83d\\udd12\"}", + "value": { + "unicode": "caf\u00e9 \u4e2d\u6587 \ud83d\udd12" + } + }, + { + "canonical": "{\"n\":0}", + "value": { + "n": 0 + } + }, + { + "canonical": "{\"n\":0}", + "value": { + "n": 0 + } + }, + { + "canonical": "{\"n\":1}", + "value": { + "n": 1 + } + }, + { + "canonical": "{\"n\":-17}", + "value": { + "n": -17 + } + }, + { + "canonical": "{\"n\":9007199254740991}", + "value": { + "n": 9007199254740991 + } + }, + { + "canonical": "{\"n\":0.5}", + "value": { + "n": 0.5 + } + }, + { + "canonical": "{\"n\":-0.25}", + "value": { + "n": -0.25 + } + }, + { + "canonical": "{\"n\":1e-07}", + "value": { + "n": 1e-07 + } + }, + { + "canonical": "{\"n\":1.5e-09}", + "value": { + "n": 1.5e-09 + } + }, + { + "canonical": "{\"links\":[{\"note\":\"\",\"relation\":\"totp\",\"target_id\":1}]}", + "value": { + "links": [ + { + "note": "", + "relation": "totp", + "target_id": 1 + } + ] + } + }, + { + "canonical": "[1,\"two\",null,true,false]", + "value": [ + 1, + "two", + null, + true, + false + ] + }, + { + "canonical": "{\"deep\":{\"a\":[{\"b\":{\"c\":[1,2,3]}}]}}", + "value": { + "deep": { + "a": [ + { + "b": { + "c": [ + 1, + 2, + 3 + ] + } + } + ] + } + } + }, + { + "canonical": "{\"archived\":false,\"custom_fields\":[],\"kind\":\"password\",\"label\":\"example.com\",\"length\":16,\"links\":[],\"modified_ts\":1700000000,\"notes\":\"\",\"tags\":[\"web\"],\"type\":\"password\"}", + "value": { + "archived": false, + "custom_fields": [], + "kind": "password", + "label": "example.com", + "length": 16, + "links": [], + "modified_ts": 1700000000, + "notes": "", + "tags": [ + "web" + ], + "type": "password" + } + } + ], + "description": "canonicalJson must match json.dumps(sort_keys=True, separators=(',',':')) exactly: entry hashes derived from it decide merge winners across implementations.", + "unsupported": [ + { + "n": 100000000000000000000 + }, + { + "n": 12345678901234567890 + } + ] +} diff --git a/js/packages/test-vectors/src/index.ts b/js/packages/test-vectors/src/index.ts index b2c16ecc..d9cdfac6 100644 --- a/js/packages/test-vectors/src/index.ts +++ b/js/packages/test-vectors/src/index.ts @@ -30,6 +30,7 @@ import entrySecretsJson from "../fixtures/entry_secrets.json"; import nostrEventsJson from "../fixtures/nostr_events.json"; import entryModsJson from "../fixtures/entry_mods.json"; import migrationsJson from "../fixtures/migrations.json"; +import canonicalJsonJson from "../fixtures/canonical_json.json"; import sshKeysJson from "../fixtures/ssh_keys.json"; import pgpKeysJson from "../fixtures/pgp_keys.json"; @@ -235,6 +236,16 @@ export const nostrEvents = nostrEventsJson as { events: { name: string; event: SignedNostrEvent }[]; }; +export interface CanonicalJsonCase { + value: unknown; + canonical: string; +} + +export const canonicalJsonCases = (canonicalJsonJson as { cases: CanonicalJsonCase[] }) + .cases; +export const canonicalJsonUnsupported = (canonicalJsonJson as { unsupported: unknown[] }) + .unsupported; + export interface SshKeyCase { mnemonic_id: string; index: number; diff --git a/scripts/canonical_json_cases.py b/scripts/canonical_json_cases.py new file mode 100644 index 00000000..21e50ea7 --- /dev/null +++ b/scripts/canonical_json_cases.py @@ -0,0 +1,102 @@ +#!/usr/bin/env python3 +"""Emit canonical-JSON differential cases from the Python reference. + +Entry hashes decide merge winners, so if the two implementations serialize +the same value differently they can pick different winners and never +converge. A security review found several such disagreements (number +formatting, key ordering) that the existing fixtures missed because they +only ever used ASCII keys and integer values. + +Writes js/packages/test-vectors/fixtures/canonical_json.json. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +OUT = REPO / "js" / "packages" / "test-vectors" / "fixtures" / "canonical_json.json" + +# Values a vault can realistically contain, plus the edges that broke. +CASES: list = [ + {}, + {"a": 1}, + {"b": 2, "a": 1}, + {"modified_ts": 1700000000, "label": "site"}, + {"tags": ["b", "a"], "notes": ""}, + # Key ordering: supplementary plane vs BMP above U+E000 + {"\U0001f600": 1, "！": 2}, + {"z": 1, "é": 2, "中": 3, "\U0001f600": 4}, + # Escaping + {"quote\"": "back\\slash"}, + {"ctrl": "\x00\x01\x1f"}, + {"nl": "line\nbreak\ttab"}, + {"unicode": "café 中文 \U0001f512"}, + # Numbers + {"n": 0}, + {"n": -0}, + {"n": 1}, + {"n": -17}, + {"n": 9007199254740991}, + {"n": 0.5}, + {"n": -0.25}, + {"n": 1e-7}, + {"n": 1.5e-9}, + # Values above 2**53 are deliberately absent: JSON gives no way to tell a + # Python int from a float there, so the two implementations cannot agree + # on a canonical form. canonicalJson refuses them rather than guessing; + # see the "unsupported" list below. + # Nesting and arrays + {"links": [{"target_id": 1, "relation": "totp", "note": ""}]}, + [1, "two", None, True, False], + {"deep": {"a": [{"b": {"c": [1, 2, 3]}}]}}, + # Entry-shaped, the actual hashed object + { + "type": "password", + "kind": "password", + "label": "example.com", + "length": 16, + "archived": False, + "notes": "", + "tags": ["web"], + "modified_ts": 1700000000, + "links": [], + "custom_fields": [], + }, +] + + +# Values TypeScript must refuse rather than serialize ambiguously. +UNSUPPORTED = [ + {"n": 10**20}, + {"n": 12345678901234567890}, +] + + +def main() -> None: + cases = [] + for value in CASES: + canonical = json.dumps(value, sort_keys=True, separators=(",", ":")) + cases.append({"value": value, "canonical": canonical}) + OUT.write_text( + json.dumps( + { + "description": ( + "canonicalJson must match json.dumps(sort_keys=True, " + "separators=(',',':')) exactly: entry hashes derived from " + "it decide merge winners across implementations." + ), + "cases": cases, + "unsupported": UNSUPPORTED, + }, + indent=2, + sort_keys=True, + ) + + "\n" + ) + print(f"wrote {OUT.relative_to(REPO)} ({len(cases)} cases)") + + +if __name__ == "__main__": + main()