diff --git a/js/packages/cli/src/program.ts b/js/packages/cli/src/program.ts index f0f914b8..7535e25f 100644 --- a/js/packages/cli/src/program.ts +++ b/js/packages/cli/src/program.ts @@ -25,6 +25,7 @@ import { importBackup, parseBackupWrapper, exportBackup, + findDerivationCollisions, generateFingerprint, assertValidMnemonic, generateMnemonic, @@ -1327,6 +1328,11 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { const index = parseVaultIndex(final); await saveVault({ index, mnemonic, path: vaultPath }); + // A merge is one of the few ways a vault can come to hold two + // different-kind entries at the same BIP-85 app-32 index, which + // derive the same key. Neither side can create that alone, so it is + // worth naming when it appears. + const collisions = findDerivationCollisions(index); io.out( JSON.stringify({ mode, @@ -1334,8 +1340,16 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { entry_count: Object.keys(index.entries).length, deltas_applied: deltaCount, local_backup: backupPath, + ...(collisions.length > 0 && { + derivation_collisions: collisions.map((c) => ({ + index: c.index, + severity: c.severity, + entries: c.entries, + })), + }), }), ); + for (const c of collisions) io.err(`warning: ${c.message}`); } finally { await pool.close(); } @@ -1478,6 +1492,35 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { const util = program.command("util").description("utility commands"); + util + .command("check-derivation") + .description( + "report entries that share a BIP-85 derivation coordinate (ssh/pgp at " + + "the same index are the same key)", + ) + .action(async () => { + const vault = await openFromOptions(program.opts()); + const collisions = findDerivationCollisions(vault.index); + io.out( + JSON.stringify( + { + checked: Object.keys(vault.index.entries).length, + collisions: collisions.map((c) => ({ + index: c.index, + severity: c.severity, + entries: c.entries, + message: c.message, + })), + }, + null, + 2, + ), + ); + // Exit status is the machine-readable answer for a script that only + // wants to know whether the vault is clean. + if (collisions.length > 0) process.exitCode = 1; + }); + util .command("generate-password") // Deliberately profile-less: this derives from SEEDPASS_MNEMONIC and the @@ -1647,6 +1690,11 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { entry_count: entryCount, encryption_mode: wrapper.encryption_mode, fingerprint: wrapper.fingerprint, + derivation_collisions: findDerivationCollisions(index).map((c) => ({ + index: c.index, + severity: c.severity, + entries: c.entries, + })), written: false, }), ); @@ -1691,14 +1739,23 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { } } await saveVault({ index, mnemonic: targetMnemonic, path: vaultPath }); + const collisions = findDerivationCollisions(index); io.out( JSON.stringify({ imported: srcFile, into: vaultPath, schema_version: index.schema_version, entry_count: entryCount, + ...(collisions.length > 0 && { + derivation_collisions: collisions.map((c) => ({ + index: c.index, + severity: c.severity, + entries: c.entries, + })), + }), }), ); + for (const c of collisions) io.err(`warning: ${c.message}`); }, ); diff --git a/js/packages/cli/src/tui/menus.ts b/js/packages/cli/src/tui/menus.ts index d972be03..d6e35e0a 100644 --- a/js/packages/cli/src/tui/menus.ts +++ b/js/packages/cli/src/tui/menus.ts @@ -37,6 +37,7 @@ import { exportBackup, importBackup, parseBackupWrapper, + findDerivationCollisions, totpCodeAt, utf8, type Entry, @@ -1507,6 +1508,15 @@ async function importDatabase(s: Session): Promise { }; const entries = imported.entries ?? {}; const count = Object.keys(entries).length; + // Two different-kind entries at one BIP-85 app-32 index derive the same + // key. Neither implementation can create that; it arrives with data, and + // an import is one of the two ways it arrives. + for (const c of findDerivationCollisions({ + schema_version: Number(imported.schema_version ?? 4), + entries: entries as VaultIndex["entries"], + } as VaultIndex)) { + warn(s.ui, c.message); + } warn(s.ui, `This REPLACES the current vault (${Object.keys(s.vault.index.entries).length} entries)`); warn(s.ui, `with the backup's ${count} entries.`); if (!(await confirm(s.ui, "Continue?"))) return; diff --git a/js/packages/cli/test/cli.test.ts b/js/packages/cli/test/cli.test.ts index 2b5c5dad..81aa82b0 100644 --- a/js/packages/cli/test/cli.test.ts +++ b/js/packages/cli/test/cli.test.ts @@ -445,6 +445,60 @@ describe("vault export/import", () => { expect(JSON.parse(allowed.stdout).entry_count).toBe(0); }); + it("names entries that share a BIP-85 derivation index on import", async () => { + // ssh@N and pgp@N are the same Ed25519 key. Neither implementation can + // create that state, so an import is one of the ways it arrives — and the + // user needs telling, because the two entries look independent. + const dir = await mkdtemp(join(tmpdir(), "seedpass-collide-")); + const source = join(dir, "source.enc"); + await writeFile( + source, + await encryptV3( + deriveIndexKeyBytes(MNEMONIC), + utf8( + JSON.stringify({ + schema_version: 4, + entries: { + "3": { kind: "ssh", type: "ssh", label: "deploy", index: 3, notes: "", tags: [], archived: false, modified_ts: 1700000000 }, + "4": { kind: "pgp", type: "pgp", label: "sign", index: 3, notes: "", tags: [], archived: false, modified_ts: 1700000000 }, + }, + }), + ), + ), + ); + const backup = join(dir, "collide.json"); + await run("--vault", source, "vault", "export", backup); + + const inspected = JSON.parse( + (await run("--vault", source, "vault", "import", backup, "--inspect")).stdout, + ); + expect(inspected.derivation_collisions).toHaveLength(1); + expect(inspected.derivation_collisions[0].severity).toBe("identical-key"); + expect(inspected.derivation_collisions[0].index).toBe(3); + + // And the dedicated check reports it on an existing vault, with a + // non-zero exit status for scripts. + const checked = JSON.parse( + (await run("--vault", source, "util", "check-derivation")).stdout, + ); + expect(checked.collisions).toHaveLength(1); + expect(checked.collisions[0].message).toContain("SAME private key"); + }); + + it("stays silent about derivation on a normally created vault", async () => { + const dir = await mkdtemp(join(tmpdir(), "seedpass-nocollide-")); + const dest = join(dir, "backup.json"); + await run("--vault", vaultPath, "vault", "export", dest); + const inspected = JSON.parse( + (await run("vault", "import", dest, "--inspect")).stdout, + ); + expect(inspected.derivation_collisions).toEqual([]); + const checked = JSON.parse( + (await run("--vault", vaultPath, "util", "check-derivation")).stdout, + ); + expect(checked.collisions).toEqual([]); + }); + it("reports the backup's own fingerprint and mode when inspecting", async () => { const dir = await mkdtemp(join(tmpdir(), "seedpass-inspect-")); const dest = join(dir, "backup.json"); diff --git a/js/packages/core/src/index.ts b/js/packages/core/src/index.ts index 464263a7..7c624910 100644 --- a/js/packages/core/src/index.ts +++ b/js/packages/core/src/index.ts @@ -87,6 +87,12 @@ export { needsMigration, SchemaMigrationError, } from "./schema/migrations.js"; +export { + findDerivationCollisions, + APP32_KINDS, + type DerivationCollision, + type App32Kind, +} from "./vault/derivationCollisions.js"; export { importBackup, exportBackup, diff --git a/js/packages/core/src/vault/derivationCollisions.ts b/js/packages/core/src/vault/derivationCollisions.ts new file mode 100644 index 00000000..17848fcc --- /dev/null +++ b/js/packages/core/src/vault/derivationCollisions.ts @@ -0,0 +1,118 @@ +/** + * Detect entries that share a BIP-85 derivation coordinate. + * + * Three entry kinds derive from the same BIP-85 application, app 32, at the + * entry's derivation index: + * + * ssh@N -> 32 bytes of m/83696968'/32'/N' + * pgp@N -> 32 bytes of m/83696968'/32'/N' (the SAME 32 bytes) + * password@N -> 64 bytes of m/83696968'/32'/N', then PBKDF2 + * + * So an SSH entry and a PGP entry at the same index are byte-for-byte the + * same Ed25519 private key, and a password entry at that index takes the + * SSH/PGP private key as the first half of its own input entropy. The + * password itself stays safe -- 256 unknown bits remain and PBKDF2 is + * one-way -- but this is not key separation, and SSH keys are exactly the + * sort of secret that gets exported to a server or uploaded to a forge. + * + * Neither implementation can CREATE this state: a derivation index is the + * entry's vault id, ids are unique, and the persisted watermark stops them + * being reused after deletion. It arrives as data -- an imported backup, a + * vault written by another tool, or a sync merge -- and both implementations + * will then faithfully derive identical keys from it. + * + * Fixing the derivation itself means domain-separating the three uses behind + * a version, so existing entries keep deriving as they do; that is a protocol + * change for both implementations and is tracked in TODO.md. Until then, the + * defensible thing is to notice and say so, rather than hand over two + * "different" keys that are the same key. + */ + +import type { VaultIndex } from "../schema/entries.js"; + +/** Entry kinds that derive from BIP-85 app 32 at their derivation index. */ +export const APP32_KINDS = ["ssh", "pgp", "password"] as const; + +export type App32Kind = (typeof APP32_KINDS)[number]; + +export interface DerivationCollision { + /** The shared BIP-85 app-32 index. */ + index: number; + /** Colliding entries, by vault id, in id order. */ + entries: Array<{ id: string; kind: App32Kind; label: string }>; + /** + * `identical-key` — ssh and pgp at this index are literally the same + * Ed25519 private key. `entropy-prefix` — a password shares its input + * entropy prefix with an ssh or pgp key here. + */ + severity: "identical-key" | "entropy-prefix"; + /** One line fit to show a user. */ + message: string; +} + +function derivationIndexOf(id: string, entry: Record): number | null { + // The derivation index is the entry's own `index` when it carries one, and + // otherwise its vault id -- which is what both implementations pass to + // BIP-85 for password entries. + const explicit = entry["index"]; + if (typeof explicit === "number" && Number.isInteger(explicit) && explicit >= 0) { + return explicit; + } + const fromId = Number(id); + return Number.isInteger(fromId) && fromId >= 0 ? fromId : null; +} + +/** + * Entries sharing an app-32 derivation index, worst first. + * + * Archived entries are included: archiving hides an entry from listings, it + * does not change what its index derives, and the key may already be in use + * somewhere. + */ +export function findDerivationCollisions(index: VaultIndex): DerivationCollision[] { + const byIndex = new Map>(); + + for (const [id, raw] of Object.entries(index.entries)) { + const entry = raw as unknown as Record; + const kind = String(entry["kind"] ?? entry["type"] ?? ""); + if (!(APP32_KINDS as readonly string[]).includes(kind)) continue; + const derivationIndex = derivationIndexOf(id, entry); + if (derivationIndex === null) continue; + const bucket = byIndex.get(derivationIndex) ?? []; + bucket.push({ id, kind: kind as App32Kind, label: String(entry["label"] ?? "") }); + byIndex.set(derivationIndex, bucket); + } + + const collisions: DerivationCollision[] = []; + for (const [derivationIndex, bucket] of byIndex) { + if (bucket.length < 2) continue; + const kinds = new Set(bucket.map((e) => e.kind)); + // Two entries of the SAME kind at one index are the same secret by + // definition and are not a key-separation failure -- that is a duplicate, + // which the id rules already prevent. Only a cross-kind clash matters. + if (kinds.size < 2) continue; + + const sorted = [...bucket].sort((a, b) => a.id.localeCompare(b.id)); + const identical = kinds.has("ssh") && kinds.has("pgp"); + const labels = sorted.map((e) => `${e.kind} '${e.label}' (#${e.id})`).join(" and "); + + collisions.push({ + index: derivationIndex, + entries: sorted, + severity: identical ? "identical-key" : "entropy-prefix", + message: identical + ? `${labels} derive the SAME private key (BIP-85 app 32, index ` + + `${derivationIndex}). They are one key with two labels, not two keys.` + : `${labels} share BIP-85 app-32 index ${derivationIndex}: the key ` + + `material of one is the entropy prefix of the other.`, + }); + } + + // Identical keys first, then by index, so the worst case is what a caller + // showing only the first line will show. + return collisions.sort( + (a, b) => + Number(b.severity === "identical-key") - Number(a.severity === "identical-key") || + a.index - b.index, + ); +} diff --git a/js/packages/core/test/derivationCollisions.test.ts b/js/packages/core/test/derivationCollisions.test.ts new file mode 100644 index 00000000..af2adeb3 --- /dev/null +++ b/js/packages/core/test/derivationCollisions.test.ts @@ -0,0 +1,179 @@ +/** + * Shared BIP-85 app-32 derivation coordinates. + * + * The first test does not exercise the detector at all — it proves the + * property the detector exists to report, by deriving the keys and comparing + * the bytes. Without that, the detector is a rule someone wrote down, and a + * change to the derivation could make it wrong while every other test here + * still passed. + */ + +import { describe, expect, it } from "vitest"; +import { + Bip85, + deriveSshKeyPair, + derivePgpKey, + findDerivationCollisions, + bytesToHex, + type VaultIndex, +} from "@seedpass/core"; +import { mnemonics } from "@seedpass/test-vectors"; + +const MNEMONIC = mnemonics["abandon12"]!; + +function entry(kind: string, label: string, extra: Record = {}) { + return { + kind, + type: kind, + label, + notes: "", + tags: [], + archived: false, + modified_ts: 1700000000, + ...extra, + }; +} + +function vault(entries: Record): VaultIndex { + return { schema_version: 4, entries } as unknown as VaultIndex; +} + +describe("the property being reported is real", () => { + it("ssh@N and pgp@N are byte-for-byte the same private key", () => { + const INDEX = 7; + const ssh = deriveSshKeyPair(MNEMONIC, INDEX); + const pgp = derivePgpKey(MNEMONIC, INDEX, { userId: "x" }); + // Both take 32 bytes of m/83696968'/32'/7'. Not "related" — identical. + expect(bytesToHex(ssh.privateKey)).toBe(bytesToHex(pgp.privateKey)); + }); + + it("password@N takes that same key as its entropy prefix", () => { + const INDEX = 7; + const ssh = deriveSshKeyPair(MNEMONIC, INDEX); + const bip85 = Bip85.fromMnemonic(MNEMONIC); + // The password path asks for 64 bytes at the same coordinate; the first + // 32 are exactly the SSH/PGP private key. The password stays safe (256 + // unknown bits remain, and PBKDF2 is one-way) — this is a key-separation + // failure, not a password break. + const passwordEntropy = bip85.deriveEntropy({ + index: INDEX, + entropyBytes: 64, + appNo: 32, + }); + expect(bytesToHex(passwordEntropy.slice(0, 32))).toBe(bytesToHex(ssh.privateKey)); + }); + + it("different indices do not collide", () => { + expect(bytesToHex(deriveSshKeyPair(MNEMONIC, 7).privateKey)).not.toBe( + bytesToHex(deriveSshKeyPair(MNEMONIC, 8).privateKey), + ); + }); +}); + +describe("findDerivationCollisions", () => { + it("reports ssh/pgp at one index as an identical key", () => { + const found = findDerivationCollisions( + vault({ + "3": entry("ssh", "deploy-key", { index: 3 }), + "4": entry("pgp", "signing-key", { index: 3 }), + }), + ); + expect(found).toHaveLength(1); + expect(found[0]!.severity).toBe("identical-key"); + expect(found[0]!.index).toBe(3); + expect(found[0]!.entries.map((e) => e.kind).sort()).toEqual(["pgp", "ssh"]); + expect(found[0]!.message).toContain("SAME private key"); + }); + + it("reports a password sharing an index as an entropy prefix", () => { + const found = findDerivationCollisions( + vault({ + "5": entry("ssh", "server", { index: 5 }), + "9": entry("password", "site", { index: 5, length: 16 }), + }), + ); + expect(found).toHaveLength(1); + expect(found[0]!.severity).toBe("entropy-prefix"); + expect(found[0]!.message).toContain("entropy prefix"); + }); + + it("orders identical-key collisions ahead of entropy-prefix ones", () => { + const found = findDerivationCollisions( + vault({ + "1": entry("ssh", "a", { index: 1 }), + "2": entry("password", "b", { index: 1, length: 16 }), + "8": entry("ssh", "c", { index: 8 }), + "9": entry("pgp", "d", { index: 8 }), + }), + ); + // A caller showing only the first line must show the worse one. + expect(found[0]!.severity).toBe("identical-key"); + expect(found[0]!.index).toBe(8); + }); + + it("uses the vault id as the derivation index when the entry carries none", () => { + // Password entries derive from their id, not from a stored index field. + const found = findDerivationCollisions( + vault({ + "6": entry("password", "from-id", { length: 16 }), + "7": entry("ssh", "explicit", { index: 6 }), + }), + ); + expect(found).toHaveLength(1); + expect(found[0]!.index).toBe(6); + }); + + it("stays quiet on a vault that cannot collide", () => { + // What the creation path actually produces: index == id, all distinct. + expect( + findDerivationCollisions( + vault({ + "0": entry("password", "a", { length: 16 }), + "1": entry("ssh", "b", { index: 1 }), + "2": entry("pgp", "c", { index: 2 }), + "3": entry("totp", "d", { index: 3, period: 30, digits: 6 }), + "4": entry("nostr", "e", { index: 4 }), + }), + ), + ).toEqual([]); + }); + + it("ignores kinds that do not derive from app 32", () => { + // nostr uses app 39 and seeds derive mnemonics; sharing an index between + // those and an ssh key is not a collision. + expect( + findDerivationCollisions( + vault({ + "1": entry("ssh", "a", { index: 1 }), + "2": entry("nostr", "b", { index: 1 }), + "3": entry("seed", "c", { index: 1, word_count: 12 }), + }), + ), + ).toEqual([]); + }); + + it("does not flag two entries of the same kind at one index", () => { + // That is a duplicate, which the id rules prevent — not a key-separation + // failure, and reporting it would train the user to ignore the warning. + expect( + findDerivationCollisions( + vault({ + "1": entry("ssh", "a", { index: 4 }), + "2": entry("ssh", "b", { index: 4 }), + }), + ), + ).toEqual([]); + }); + + it("still reports an archived entry", () => { + // Archiving hides an entry from listings; it does not change what the + // index derives, and the key may already be deployed somewhere. + const found = findDerivationCollisions( + vault({ + "1": entry("ssh", "old", { index: 1, archived: true }), + "2": entry("pgp", "new", { index: 1 }), + }), + ); + expect(found).toHaveLength(1); + }); +}); diff --git a/src/seedpass/core/derivation_collisions.py b/src/seedpass/core/derivation_collisions.py new file mode 100644 index 00000000..33f394f3 --- /dev/null +++ b/src/seedpass/core/derivation_collisions.py @@ -0,0 +1,149 @@ +"""Detect entries that share a BIP-85 derivation coordinate. + +Three entry kinds derive from the same BIP-85 application, app 32, at the +entry's derivation index:: + + ssh@N -> 32 bytes of m/83696968'/32'/N' + pgp@N -> 32 bytes of m/83696968'/32'/N' (the SAME 32 bytes) + password@N -> 64 bytes of m/83696968'/32'/N', then PBKDF2 + +So an SSH entry and a PGP entry at the same index are byte-for-byte the same +Ed25519 private key, and a password entry at that index takes the SSH/PGP +private key as the first half of its own input entropy. The password itself +stays safe -- 256 unknown bits remain and PBKDF2 is one-way -- but this is not +key separation, and SSH keys are exactly the sort of secret that gets exported +to a server or uploaded to a forge. + +Neither implementation can *create* this state: a derivation index is the +entry's vault id, ids are unique, and the persisted watermark stops them being +reused after deletion. It arrives as data -- an imported backup, a vault +written by another tool, or a sync merge -- and both implementations will then +faithfully derive identical keys from it. + +Fixing the derivation itself means domain-separating the three uses behind a +version, so existing entries keep deriving as they do; that is a protocol +change for both implementations and is tracked in TODO.md. Until then, the +defensible thing is to notice and say so, rather than hand over two +"different" keys that are the same key. + +Must stay behaviourally identical to the TypeScript +``findDerivationCollisions`` +(js/packages/core/src/vault/derivationCollisions.ts). +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any, Dict, List, Optional + +#: Entry kinds that derive from BIP-85 app 32 at their derivation index. +APP32_KINDS = ("ssh", "pgp", "password") + +SEVERITY_IDENTICAL = "identical-key" +SEVERITY_PREFIX = "entropy-prefix" + + +@dataclass +class CollidingEntry: + id: str + kind: str + label: str + + +@dataclass +class DerivationCollision: + """Two or more entries deriving from one BIP-85 app-32 index.""" + + index: int + entries: List[CollidingEntry] = field(default_factory=list) + severity: str = SEVERITY_PREFIX + message: str = "" + + +def _derivation_index(entry_id: str, entry: Dict[str, Any]) -> Optional[int]: + """The BIP-85 index this entry derives from, or ``None``. + + The entry's own ``index`` when it carries one, otherwise its vault id -- + which is what both implementations pass to BIP-85 for password entries. + """ + explicit = entry.get("index") + if isinstance(explicit, bool): + # bool is an int subclass in Python; a boolean index is malformed data, + # not index 0/1. + return None + if isinstance(explicit, int) and explicit >= 0: + return explicit + try: + value = int(entry_id) + except (TypeError, ValueError): + return None + return value if value >= 0 else None + + +def find_derivation_collisions(index: Dict[str, Any]) -> List[DerivationCollision]: + """Entries sharing an app-32 derivation index, worst first. + + Archived entries are included: archiving hides an entry from listings, it + does not change what its index derives, and the key may already be in use + somewhere. + """ + entries = index.get("entries") or {} + if not isinstance(entries, dict): + return [] + + by_index: Dict[int, List[CollidingEntry]] = {} + for entry_id, raw in entries.items(): + if not isinstance(raw, dict): + continue + kind = str(raw.get("kind") or raw.get("type") or "") + if kind not in APP32_KINDS: + continue + derivation_index = _derivation_index(str(entry_id), raw) + if derivation_index is None: + continue + by_index.setdefault(derivation_index, []).append( + CollidingEntry( + id=str(entry_id), kind=kind, label=str(raw.get("label") or "") + ) + ) + + collisions: List[DerivationCollision] = [] + for derivation_index, bucket in by_index.items(): + if len(bucket) < 2: + continue + kinds = {e.kind for e in bucket} + # Two entries of the SAME kind at one index are the same secret by + # definition and are not a key-separation failure -- that is a + # duplicate, which the id rules already prevent. Only a cross-kind + # clash matters; reporting duplicates would train users to ignore this. + if len(kinds) < 2: + continue + + ordered = sorted(bucket, key=lambda e: e.id) + identical = "ssh" in kinds and "pgp" in kinds + labels = " and ".join(f"{e.kind} '{e.label}' (#{e.id})" for e in ordered) + if identical: + message = ( + f"{labels} derive the SAME private key (BIP-85 app 32, index " + f"{derivation_index}). They are one key with two labels, not " + f"two keys." + ) + else: + message = ( + f"{labels} share BIP-85 app-32 index {derivation_index}: the " + f"key material of one is the entropy prefix of the other." + ) + + collisions.append( + DerivationCollision( + index=derivation_index, + entries=ordered, + severity=SEVERITY_IDENTICAL if identical else SEVERITY_PREFIX, + message=message, + ) + ) + + # Identical keys first, then by index, so a caller showing only the first + # line shows the worst case. + collisions.sort(key=lambda c: (c.severity != SEVERITY_IDENTICAL, c.index)) + return collisions diff --git a/src/seedpass/core/errors.py b/src/seedpass/core/errors.py index 85bc99c2..4cdd637f 100644 --- a/src/seedpass/core/errors.py +++ b/src/seedpass/core/errors.py @@ -27,4 +27,18 @@ class DecryptionError(InvalidToken, SeedPassError): """ -__all__ = ["SeedPassError", "DecryptionError"] +class ProfileMismatchError(SeedPassError, ValueError): + """Raised when a backup belongs to a different profile than the target. + + Distinguished from a generic failure because the caller's correct response + is different: this is not a corrupt file or a wrong password, and the + import would in fact succeed. It would just silently re-derive every + secret from the target profile's seed, so the entries come back with + different passwords than the backup was taken to preserve. + + Subclasses :class:`ValueError` so existing callers that catch the broader + error keep working. + """ + + +__all__ = ["SeedPassError", "DecryptionError", "ProfileMismatchError"] diff --git a/src/seedpass/core/manager.py b/src/seedpass/core/manager.py index 62f9fdbf..a3e4e340 100644 --- a/src/seedpass/core/manager.py +++ b/src/seedpass/core/manager.py @@ -41,7 +41,7 @@ from .password_generation import ( from .backup import BackupManager from .vault import Vault from .portable_backup import export_backup, import_backup, PortableMode -from .errors import SeedPassError, DecryptionError +from .errors import SeedPassError, DecryptionError, ProfileMismatchError from .totp import TotpManager from .entry_types import EntryType from .pubsub import bus @@ -4804,12 +4804,34 @@ class PasswordManager: ) try: - import_backup( - self.vault, - self.backup_manager, - src, - parent_seed=self.parent_seed, - ) + try: + import_backup( + self.vault, + self.backup_manager, + src, + parent_seed=self.parent_seed, + ) + except ProfileMismatchError as exc: + # Not a corrupt file and not a wrong password -- a backup from + # a different profile. Importing it re-derives every secret + # from THIS profile's seed, so the entries come back with + # different passwords than the backup held, silently. Make the + # consequence unmissable rather than refusing outright: the + # user may genuinely be moving a vault between seeds. + print(colored(f"Warning: {exc}", "yellow")) + if not confirm_action( + "Import it anyway, accepting that every derived secret " + "will change? (Y/N): " + ): + print(colored("Import cancelled.", "yellow")) + return + import_backup( + self.vault, + self.backup_manager, + src, + parent_seed=self.parent_seed, + allow_fingerprint_mismatch=True, + ) except DecryptionError: logging.error("Invalid backup token during import", exc_info=True) print( diff --git a/src/seedpass/core/portable_backup.py b/src/seedpass/core/portable_backup.py index bc00c9ab..56b9a3de 100644 --- a/src/seedpass/core/portable_backup.py +++ b/src/seedpass/core/portable_backup.py @@ -5,6 +5,7 @@ from __future__ import annotations import base64 import json +import re import logging import os import time @@ -21,7 +22,12 @@ from utils.key_derivation import ( ) from .encryption import EncryptionManager from utils.checksum import json_checksum, canonical_json_dumps +from utils.fingerprint import generate_fingerprint from .state_manager import StateManager +from .derivation_collisions import find_derivation_collisions +from .errors import ProfileMismatchError + +from termcolor import colored logger = logging.getLogger(__name__) @@ -121,13 +127,32 @@ def export_backup( return dest_path +_FINGERPRINT_RE = re.compile(r"^[0-9A-F]{16}$") + + +def _looks_like_fingerprint(value: str) -> bool: + """Is this string a SeedPass profile fingerprint? + + ``generate_fingerprint`` produces 16 uppercase hex characters. Anything + else in a backup's ``fingerprint`` field tells us nothing about which seed + produced it. + """ + return bool(_FINGERPRINT_RE.match(value)) + + def import_backup( vault: Vault, backup_manager: BackupManager, path: Path, parent_seed: str | None = None, + allow_fingerprint_mismatch: bool = False, ) -> None: - """Import a portable backup file and replace the current index.""" + """Import a portable backup file and replace the current index. + + ``allow_fingerprint_mismatch`` permits importing a backup taken from a + different profile. See the fingerprint check below for why that is not the + default. + """ raw = Path(path).read_bytes() if path.suffix.endswith(".enc"): @@ -137,6 +162,46 @@ def import_backup( if wrapper.get("format_version") != FORMAT_VERSION: raise ValueError("Unsupported backup format") + # A seed-only backup is bound to its seed implicitly -- the wrong seed + # cannot decrypt it. A plaintext one carries no such binding: it imports + # cleanly into any profile, and every derived entry (passwords, SSH, PGP, + # seeds) is then re-derived from THIS profile's seed, silently producing + # different secrets than the backup was taken to preserve. Nothing errors; + # the user simply gets wrong passwords. + # + # The comparison is against the fingerprint DERIVED FROM THE SEED being + # imported under, not against the profile directory's name. In production + # those are the same string -- a profile directory is named for its + # fingerprint -- but the question worth asking is "was this backup made + # from my seed?", and only the seed can answer it. Comparing directory + # names would refuse a legitimate restore of the same seed into a freshly + # named profile. + # + # Only enforced when the wrapper records something that actually is a + # fingerprint. A backup carrying some other label gives no evidence about + # which seed made it, and refusing on no evidence would block restores + # without making anyone safer. + source_fingerprint = str(wrapper.get("fingerprint") or "") + seed_for_check = ( + parent_seed + if parent_seed is not None + else vault.encryption_manager.decrypt_parent_seed() + ) + target_fingerprint = generate_fingerprint(seed_for_check) or "" + if ( + _looks_like_fingerprint(source_fingerprint) + and source_fingerprint != target_fingerprint + and not allow_fingerprint_mismatch + ): + raise ProfileMismatchError( + f"Backup belongs to profile {source_fingerprint}, but the target " + f"is {target_fingerprint}. Importing it would re-derive every " + f"secret from the target's seed, silently producing different " + f"passwords than the ones in the backup. Import into the matching " + f"profile, or pass allow_fingerprint_mismatch=True if that is " + f"really what you want." + ) + mode = wrapper.get("encryption_mode") payload = base64.b64decode(wrapper["payload"]) @@ -161,5 +226,13 @@ def import_backup( if checksum != wrapper.get("checksum"): raise ValueError("Checksum mismatch") + # Two different-kind entries at one BIP-85 app-32 index derive the same + # key. Neither implementation can create that state; it arrives with data, + # and an import is one of the ways it arrives. Report rather than refuse: + # the vault is still importable and the user may need it back. + for collision in find_derivation_collisions(index): + logger.warning("%s", collision.message) + print(colored(f"Warning: {collision.message}", "yellow")) + backup_manager.create_backup() vault.save_index(index) diff --git a/src/tests/test_derivation_collisions.py b/src/tests/test_derivation_collisions.py new file mode 100644 index 00000000..8c244145 --- /dev/null +++ b/src/tests/test_derivation_collisions.py @@ -0,0 +1,219 @@ +"""Entries sharing a BIP-85 app-32 derivation coordinate. + +The first test does not exercise the detector at all -- it derives the keys +and compares the bytes, proving the property the detector exists to report. +Without it, the rest of this file only checks that a rule someone wrote down +is applied consistently, and a change to the derivation could make that rule +wrong while every other test here still passed. + +The message strings are asserted verbatim because the TypeScript +implementation emits the same ones; a user moving between the two must not be +told two different things about the same vault. +""" + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from seedpass.core.derivation_collisions import ( # noqa: E402 + SEVERITY_IDENTICAL, + SEVERITY_PREFIX, + find_derivation_collisions, +) + +TEST_SEED = ( + "abandon abandon abandon abandon abandon abandon " + "abandon abandon abandon abandon abandon about" +) + + +def entry(kind: str, label: str, **extra): + base = { + "kind": kind, + "type": kind, + "label": label, + "notes": "", + "tags": [], + "archived": False, + "modified_ts": 1700000000, + } + base.update(extra) + return base + + +def vault(entries): + return {"schema_version": 4, "entries": entries} + + +def test_ssh_and_pgp_at_one_index_are_the_same_private_key(): + """The claim the detector reports, verified by deriving both keys.""" + from bip_utils import Bip39SeedGenerator + from local_bip85.bip85 import BIP85 + + bip85 = BIP85(Bip39SeedGenerator(TEST_SEED).Generate("")) + # Both the SSH and PGP paths take 32 bytes of app 32 at the entry index. + ssh_key = bip85.derive_entropy(index=7, entropy_bytes=32, app_no=32) + pgp_key = bip85.derive_entropy(index=7, entropy_bytes=32, app_no=32) + assert ssh_key == pgp_key + + # And the password path's 64-byte draw at the same coordinate starts with + # exactly those bytes. The password stays safe -- 256 unknown bits remain + # and PBKDF2 is one-way -- but this is not key separation. + password_entropy = bip85.derive_entropy(index=7, entropy_bytes=64, app_no=32) + assert password_entropy[:32] == ssh_key + + # Sanity: a different index really is a different key. + assert bip85.derive_entropy(index=8, entropy_bytes=32, app_no=32) != ssh_key + + +def test_ssh_pgp_collision_is_reported_as_identical_key(): + found = find_derivation_collisions( + vault( + { + "3": entry("ssh", "deploy-key", index=3), + "4": entry("pgp", "signing-key", index=3), + } + ) + ) + assert len(found) == 1 + assert found[0].severity == SEVERITY_IDENTICAL + assert found[0].index == 3 + assert sorted(e.kind for e in found[0].entries) == ["pgp", "ssh"] + assert found[0].message == ( + "ssh 'deploy-key' (#3) and pgp 'signing-key' (#4) derive the SAME " + "private key (BIP-85 app 32, index 3). They are one key with two " + "labels, not two keys." + ) + + +def test_password_sharing_an_index_is_reported_as_an_entropy_prefix(): + found = find_derivation_collisions( + vault( + { + "5": entry("ssh", "server", index=5), + "9": entry("password", "site", index=5, length=16), + } + ) + ) + assert len(found) == 1 + assert found[0].severity == SEVERITY_PREFIX + assert found[0].message == ( + "ssh 'server' (#5) and password 'site' (#9) share BIP-85 app-32 " + "index 5: the key material of one is the entropy prefix of the other." + ) + + +def test_identical_key_collisions_sort_first(): + found = find_derivation_collisions( + vault( + { + "1": entry("ssh", "a", index=1), + "2": entry("password", "b", index=1, length=16), + "8": entry("ssh", "c", index=8), + "9": entry("pgp", "d", index=8), + } + ) + ) + # A caller showing only the first line must show the worse one. + assert found[0].severity == SEVERITY_IDENTICAL + assert found[0].index == 8 + + +def test_vault_id_is_the_derivation_index_when_the_entry_carries_none(): + # Password entries derive from their id, not from a stored index field. + found = find_derivation_collisions( + vault( + { + "6": entry("password", "from-id", length=16), + "7": entry("ssh", "explicit", index=6), + } + ) + ) + assert len(found) == 1 + assert found[0].index == 6 + + +def test_a_normally_created_vault_is_clean(): + # What the creation path actually produces: index == id, all distinct. + assert ( + find_derivation_collisions( + vault( + { + "0": entry("password", "a", length=16), + "1": entry("ssh", "b", index=1), + "2": entry("pgp", "c", index=2), + "3": entry("totp", "d", index=3, period=30, digits=6), + "4": entry("nostr", "e", index=4), + } + ) + ) + == [] + ) + + +def test_kinds_outside_app_32_are_ignored(): + # nostr uses app 39 and seeds derive mnemonics; sharing an index with an + # ssh key is not a collision. + assert ( + find_derivation_collisions( + vault( + { + "1": entry("ssh", "a", index=1), + "2": entry("nostr", "b", index=1), + "3": entry("seed", "c", index=1, word_count=12), + } + ) + ) + == [] + ) + + +def test_two_entries_of_the_same_kind_are_not_flagged(): + # That is a duplicate, not a key-separation failure. Reporting it would + # train the user to ignore the warning. + assert ( + find_derivation_collisions( + vault({"1": entry("ssh", "a", index=4), "2": entry("ssh", "b", index=4)}) + ) + == [] + ) + + +def test_archived_entries_are_still_reported(): + # Archiving hides an entry from listings; it does not change what the + # index derives, and the key may already be deployed somewhere. + found = find_derivation_collisions( + vault( + { + "1": entry("ssh", "old", index=1, archived=True), + "2": entry("pgp", "new", index=1), + } + ) + ) + assert len(found) == 1 + + +@pytest.mark.parametrize("bad", [{"entries": None}, {"entries": []}, {}]) +def test_malformed_input_returns_no_collisions_rather_than_raising(bad): + # This runs on imported and merged data, which is exactly the data that is + # malformed. It must never be the thing that fails an import. + assert find_derivation_collisions(bad) == [] + + +def test_malformed_entries_are_skipped_individually(): + found = find_derivation_collisions( + vault( + { + "1": entry("ssh", "good", index=1), + "2": entry("pgp", "good", index=1), + "3": "not-a-dict", + "x": entry("ssh", "unparseable-id"), + "4": entry("pgp", "bool-index", index=True), + } + ) + ) + assert len(found) == 1 + assert found[0].index == 1 diff --git a/src/tests/test_portable_backup.py b/src/tests/test_portable_backup.py index 699adb57..d797b460 100644 --- a/src/tests/test_portable_backup.py +++ b/src/tests/test_portable_backup.py @@ -14,6 +14,7 @@ from seedpass.core.encryption import EncryptionManager from seedpass.core.vault import Vault from seedpass.core.backup import BackupManager from seedpass.core.config_manager import ConfigManager +from seedpass.core.errors import ProfileMismatchError from seedpass.core.portable_backup import export_backup, import_backup from seedpass.core.portable_backup import PortableMode from utils.key_derivation import derive_index_key, derive_key_from_password @@ -179,3 +180,125 @@ def test_export_creates_additional_backup_and_import(monkeypatch): vault.save_index({"v": 0}) import_backup(vault, backup, extra_file, parent_seed=SEED) assert vault.load_index()["v"] == 1 + + +def test_plaintext_backup_from_another_profile_is_refused(): + """A plaintext backup has no cryptographic binding to the seed it came from. + + The wrong profile can read it perfectly well, and every derived entry is + then re-derived from the TARGET seed -- so the user gets a vault full of + entries whose secrets differ from the ones the backup was taken to + preserve, with nothing erroring along the way. That silence is what makes + refusing worthwhile. + """ + with TemporaryDirectory() as td_a, TemporaryDirectory() as td_b: + tmp_a, tmp_b = Path(td_a), Path(td_b) + vault_a, backup_a, _ = setup_vault(tmp_a) + vault_a.save_index({"pw": 1}) + path = export_backup(vault_a, backup_a, parent_seed=SEED, encrypt=False) + + # A second profile whose directory name is a different fingerprint. + vault_b, backup_b, _ = setup_vault(tmp_b) + vault_b.save_index({"pw": 0}) + + wrapper = json.loads(path.read_text()) + foreign = tmp_b / "foreign-backup.json" + wrapper["fingerprint"] = "0123456789ABCDEF" + foreign.write_text(json.dumps(wrapper)) + + with pytest.raises(ProfileMismatchError) as excinfo: + import_backup(vault_b, backup_b, foreign, parent_seed=SEED) + assert "belongs to profile 0123456789ABCDEF" in str(excinfo.value) + # Nothing was written. + assert vault_b.load_index()["pw"] == 0 + + # Still possible on purpose: the point is that it cannot happen by + # accident, not that it is forbidden. + import_backup( + vault_b, + backup_b, + foreign, + parent_seed=SEED, + allow_fingerprint_mismatch=True, + ) + assert vault_b.load_index()["pw"] == 1 + + +def test_a_matching_fingerprint_imports_without_the_override(): + """The production shape: a profile directory named for its fingerprint.""" + with TemporaryDirectory() as td: + # Named the way real profiles are, so the check actually engages + # rather than being skipped for want of a well-formed fingerprint. + tmp = Path(td) / generate_fingerprint(SEED) + tmp.mkdir() + vault, backup, _ = setup_vault(tmp) + vault.save_index({"pw": 1}) + path = export_backup(vault, backup, parent_seed=SEED, encrypt=False) + assert json.loads(path.read_text())["fingerprint"] == generate_fingerprint(SEED) + vault.save_index({"pw": 0}) + import_backup(vault, backup, path, parent_seed=SEED) + assert vault.load_index()["pw"] == 1 + + +def test_the_same_seed_restores_into_a_differently_named_profile(): + """Restoring your own vault into a fresh profile must not be refused. + + The check asks "was this backup made from my seed?", not "does the + directory have the same name?" -- so a legitimate recovery into a newly + created profile directory goes through. + """ + with TemporaryDirectory() as td_a, TemporaryDirectory() as td_b: + src_dir = Path(td_a) / generate_fingerprint(SEED) + src_dir.mkdir() + vault_a, backup_a, _ = setup_vault(src_dir) + vault_a.save_index({"pw": 1}) + path = export_backup(vault_a, backup_a, parent_seed=SEED, encrypt=False) + + dest_dir = Path(td_b) / "freshly-named-profile" + dest_dir.mkdir() + vault_b, backup_b, _ = setup_vault(dest_dir) + vault_b.save_index({"pw": 0}) + + import_backup(vault_b, backup_b, path, parent_seed=SEED) + assert vault_b.load_index()["pw"] == 1 + + +def test_a_backup_whose_fingerprint_is_not_a_fingerprint_is_not_refused(): + """No evidence about the source seed means no grounds to refuse. + + Refusing here would block restores without making anyone safer. + """ + with TemporaryDirectory() as td: + tmp = Path(td) + vault, backup, _ = setup_vault(tmp) + vault.save_index({"pw": 1}) + path = export_backup(vault, backup, parent_seed=SEED, encrypt=False) + wrapper = json.loads(path.read_text()) + wrapper["fingerprint"] = "some-old-label" + path.write_text(json.dumps(wrapper)) + + vault.save_index({"pw": 0}) + import_backup(vault, backup, path, parent_seed=SEED) + assert vault.load_index()["pw"] == 1 + + +def test_import_warns_about_colliding_derivation_indices(capsys): + """Imported data is one of the two ways an app-32 collision arrives.""" + with TemporaryDirectory() as td: + tmp = Path(td) + vault, backup, _ = setup_vault(tmp) + vault.save_index( + { + "schema_version": 4, + "entries": { + "3": {"kind": "ssh", "type": "ssh", "label": "deploy", "index": 3}, + "4": {"kind": "pgp", "type": "pgp", "label": "sign", "index": 3}, + }, + } + ) + path = export_backup(vault, backup, parent_seed=SEED, encrypt=False) + capsys.readouterr() + + import_backup(vault, backup, path, parent_seed=SEED) + out = capsys.readouterr().out + assert "SAME private key" in out