peach-browser @ 13c7d70db28a

peach-code-review.md

32.2 KB · raw

rendered · source

Peach Browser Extension — Independent Code Review

Reviewer: Claude Fable 5 Date: 2026-06-12 Codebase: peach-browser (v4.0.0) Method: Complete static read of all security-relevant source files

Overview

Peach is a local-first, zero-knowledge browser password manager (Manifest V3, Chrome + Firefox, plus a native Android port that is out of scope here). The vault is encrypted on-device with AES-256-GCM under an Argon2id-derived key; no plaintext ever leaves the machine. Beyond storage it implements browser autofill, WebAuthn/passkey provisioning, a Pro-gated LAN device-sync feature (audio "chirp" pairing → Noise-encrypted WebSocket), a paper backup system ("Codex": Reed-Solomon-coded Aztec matrices rendered to PDF), and a duress/decoy two-slot vault.

The dominant impression from reading the code is that it has been through serious, repeated security review and that the fixes were integrated thoughtfully rather than bolted on. Nearly every security-relevant decision carries an inline rationale tied to a specific finding ID (audit HV-*/MV-*/ LV-*, technical-report PCH-01-*). Cryptographic primitives are standard and correctly parameterised. The extension's trust boundaries — content script vs. background, page origin vs. vault — are enforced with browser-attested signals rather than payload-supplied ones. Error handling consistently fails closed for secrets and fails gracefully for availability.

The findings below are, with one exception, Informational or Low. I found no Critical or High issues. This is consistent with a codebase that has already absorbed two prior review rounds; what remains is mostly defense-in-depth hardening, documented residual risks, and a small number of asymmetries worth tightening.

Scope

Read completely:

Scope deviations: three brief paths do not exist as written. AES-GCM lives in src/background/aes.ts and inline in peachPayload.ts (a header comment in aes.ts:4 records that a duplicate src/crypto/aes.ts was removed); the Noise NK pattern is in noise-js.ts; the Codex erasure module is in src/popup/codex/. All three were located and read.

Cryptographic Implementation

Argon2id (master-password KDF) — argon2.ts, also inline in peachPayload.ts and as parameters in codex-types. m = 65536 KiB (64 MiB), t = 3, p = 4, 32-byte output, via hash-wasm. This meets the OWASP password-manager tier and exceeds it on parallelism. The password is NFC-normalised before derivation (argon2.ts:32) so the same passphrase typed under different Unicode normalisation forms (macOS NFD dead-keys vs. mobile NFC) derives the same key and stays cross-platform-compatible; a deriveKeyRaw legacy path (argon2.ts:40-45) exists solely to open vaults sealed before that fix. The salt is 16 bytes from crypto.getRandomValues, fresh at creation and on every password change (argon2.ts:50-52). Appropriate and correct.

AES-256-GCM (vault-at-rest and export envelope) — aes.ts, peachPayload.ts:997-1051. 96-bit random nonce per encryption (aes.ts:56), 128-bit tag (the maximum, explicitly pinned and commented as non-reducible at aes.ts:51-55). Keys are imported non-extractable, used once, then zeroed by callers (vault.ts zeroes derivedKey/rootKey on lock and after key swaps). The AAD carries a per-slot domain tag ("slot_a_v1", "slot_b_v1", "pw_root_v1") with a legacy single-byte-AAD fallback on decrypt so pre-hardening vaults still open (aes.ts:104-129). The construction is textbook and the nonce-reuse hazard is correctly avoided with fresh randomness rather than a counter. Note that GCM's ~2³² safe-message bound per key is irrelevant here — a vault is re-encrypted as a single message per write.

BLAKE3 (payload checksum, vault id, card fingerprints) — via hash-wasm. In peachPayload.ts a 4-byte truncated BLAKE3 checksum sits inside the encrypted envelope (blake3Truncated, 1053-1061) purely to fail fast on compressed-payload corruption before decompression — authenticity is GCM's job, not the checksum's, and the code says so (peachPayload.ts:27-29). The checksum comparison is constant-time (1063-1068). computeVaultId is BLAKE3(masterKey || "peach-vault-id") (sync-identity.ts:68-75).

zstd / pako (compression) — zstd.ts, generate.ts. Compression is not a security primitive but is a decompression-bomb surface; it is bounded (see PCH-02-008 context and import-limits.ts).

X25519 + ChaCha20-Poly1305 + BLAKE2s (Noise XX / NK) — noise-xx.ts, noise-js.ts. Both patterns (Noise_XX_25519_ChaChaPoly_BLAKE2s for mutual-auth WebSocket sync, Noise_NK_… for post-chirp pairing) derive the protocol-name init hash and the HKDF chain over BLAKE2s, matching the named pattern (a prior round had used SHA-256 here; PCH-01-002 corrected it). Because @stablelib's HKDF reuses one HMAC across output blocks and BLAKE2s's restoreState leaves the finished flag set, the code runs HKDF-Expand manually with a fresh HMAC-BLAKE2s per block (noise-xx.ts:46-58), documented as byte-identical to the Android BouncyCastle side. Transport keys are split correctly and direction-swapped on the responder; nonces are per-frame counters from 0. The XX initiator pins the responder's static key before transmitting its own static in message 3 (noise-xx.ts:78-133, PCH-01-003), so a probed-but-wrong LAN host never learns the prober's long-term identity.

Ed25519 (device-manifest signing) — sync-identity.ts:89-161, device-manifest.ts. Deterministically derived from the root key under a distinct HKDF salt ("peach-sync-signing-v1") so the signing key is single-purpose and survives reinstall. Manifests are signed over a canonical JSON encoding (sorted keys, no insignificant whitespace, non-finite numbers rejected — device-manifest.ts:183-204) and verified with strict per-device field validation and a freshness window. The private key is imported as extractable, which the inline comment justifies and PCH-01-022 documents: the 32-byte seed is already in memory, so a JWK export to recover the public key adds no attack surface.

HKDF-SHA256 (sync identity, recovery KEK, per-entry sync keys) — sync-identity.ts, recovery-words.ts:130-155, chirp-sync-vault-adapter.ts:355-369. Each use is domain-separated. The per-entry sync envelope key is HKDF(rootKey, info = entryId + "\x00" + "sync-entry-v1"); the null-byte separator makes the (id, suffix) boundary unambiguous (PCH-01-005), and the adapter rejects any decrypted envelope whose entry.id does not equal the map key it arrived under, so a ciphertext minted for one slot cannot be replayed into another (chirp-sync-vault-adapter.ts:153-155).

PBKDF2-SHA256 (legacy export, self-test) — backup.ts:146-173 derives the key for the legacy encrypted-JSON import format with PBKDF2-SHA256 at 250,000 iterations. self-test.ts uses PBKDF2 at 1,000 iterations deliberately, only as a fast Known-Answer-Test vehicle on the same AES-GCM path (it pins the expected ciphertext, PCH-01-020, so a self-consistent-but-broken WebCrypto fails). See PCH-02-005.

BIP39 recovery words — recovery-words.ts. A 12-word phrase encodes a 128-bit prefix of the 256-bit root key plus a 4-bit SHA-256 checksum, exactly per BIP39. The code is explicit and repeated about the security model (recovery-words.ts:20-26, 116-120, vault.ts:282-296): recovery is 128-bit, the words wrap the full root key via an HKDF-derived KEK from a prefix of that key, and a future 256-bit-recovery scheme would need independent entropy. This is a correct and unusually well-documented construction.

TOTP — totp.ts delegates to the otpauth library; the entry's stored algorithm/digits/period are threaded through (PCH-01-019) with RFC defaults (SHA-1/6/30) when absent. The raw base32 seed never crosses to a content script (see Extension boundary); the background generates the current code and ships only that plus seconds-remaining.

PeachScript — peachScript.ts is a trivial byte → Unicode-PUA codec (U+E000…U+E0FF) for the device-to-device export string. It is presentation, not cryptography — the payload it carries is already an AES-GCM envelope — and the file says so. Decode validates every code point is in range.

Code Review Findings

Finding PCH-02-001: Update-entry patch path bypasses the canary/tags/passkey sanitizers used on add and restore

Severity: Low Location: src/background/vault.ts:2317-2417 (sanitizeEntryPatch), src/background/messages.ts:478-501 (UPDATE_ENTRY handler) Description: Entries entering the vault via add (sanitizeEntryDraft, vault.ts:2256-2315), restore, or sync (sanitizeRestoredEntry, vault.ts:1603-1650) are run through field whitelists that rebuild passkey and canary sub-objects field-by-field and bound tags (sanitizePasskey/sanitizeCanary/sanitizeTags). The UPDATE_ENTRY path does not: sanitizeEntryPatch copies patch.canary (2375-2377), patch.tags (2386-2388), and patch.passkey (2379-2384) through verbatim. UPDATE_ENTRY is also not in DESTRUCTIVE_MESSAGE_TYPES (messages.ts:141-146), so unlike DESTROY_VAULT/RESTORE_* it is not gated to extension-page senders — and it is legitimately sent from content-script context (the inline editor and the canary-ping bump at content/index.ts:336-348 and content/save.ts:57-61), so it cannot simply be gated. The net effect is that a caller able to send an UPDATE_ENTRY for a known entry id could attach an arbitrary passkey sub-object, set unbounded tags, or replace a canary object on an entry that already has one without the field rebuild the other paths enforce. Notes: Exploitability is genuinely low: web pages cannot call chrome.runtime.sendMessage (no externally_connectable), so this requires compromising the content script itself, and Peach's injected UI mounts in closed shadow roots (content/index.ts:164, inject.ts attachShadow({mode:"closed"}) throughout), which removes the obvious in-page foothold. Adding a new canary is separately Pro-gated (messages.ts:491-498) and the canary fire path re-validates the webhook with the full SSRF guard (isAllowedCanaryWebhookUrl, messages.ts:1568-1600). The cleanest fix is to route sanitizeEntryPatch's canary/tags/passkey branches through the same sanitizers the other paths use; the change is local and does not alter the trust boundary.

Finding PCH-02-002: Noise XX message 1 omits the canonical empty-payload hash step

Severity: Informational Location: src/shared/noise-xx.ts:93-97 (initiator), 184-191 (responder) Description: In a canonical Noise handshake, every message — including the first -> e — ends by processing its payload through EncryptAndHash, which for an empty payload with no symmetric key yet still performs MixHash(empty). The Peach XX implementation processes the ephemeral (mixHash(h, e.publicKey)) and sends it, but does not perform the empty-payload MixHash for message 1. The NK implementation does hash its message payloads (noise-js.ts:90-91, 112-116), because by those points a key exists and a real AEAD tag is produced. The consequence is that Peach's XX transcript hash diverges by one MixHash(empty) from what a stock Noise library (e.g. snow) would compute, so a third-party Noise stack would not interoperate with Peach's XX handshake. Notes: This is not a confidentiality or authenticity weakness: both Peach peers (and the Android hand-port) compute h identically, the handshake hash still binds the full transcript that both sides observe, and the transport keys derive from the chaining key, not h. The only practical effect is interop with external implementations, which Peach does not rely on (the doc comment at noise-js.ts:11-14 notes interop is held "by porting discipline, not by a shared library"). The header comment in noise-js.ts:12 that "a stock snow stack would interoperate" holds for NK but is not strictly true for XX; worth a one-line correction so the claim is not over-read.

Finding PCH-02-003: Pairing authentication rests on the user-confirmed SAS and an unverifiable peer-side gate

Severity: Informational Location: src/shared/chirp-handshake.ts:125-216 Description: The post-chirp pairing handshake uses Noise NK, which authenticates only the responder (it proves the initiator encrypted to the responder's chirp-broadcast ephemeral). It does not cryptographically authenticate the initiator, and there is no proof-of-possession over the long_term_public_key_hex a peer advertises in its PAIR_IDENTITY frame. The compensating control is a 6-digit Short Authentication String derived from the shared Noise handshake hash (derivePairingSAS, 163-174), which the extension pairing flow requires the user to confirm matches the peer's before any device is persisted (gateOnSAS, referenced as living in src/popup/chirp-pair.ts). Notes: This is called out thoroughly and honestly in the source itself (chirp-handshake.ts:125-154) — one of the better in-code threat statements in the codebase. Residual risk is the standard SAS bound: a MITM/relay survives with probability ~1e-6 per attempt and only if the user does not actually compare the codes. The open item the code names — that the Android/native peer must enforce the identical SAS gate before persisting — cannot be verified from this repository and is the more important dependency: if the app auto-confirms, the channel is only as strong as its weakest end. Recommend confirming the app-side gate against the Android source, and (as the comment suggests) considering a future proof-of-possession round via the already-present performInitiatorAuth/performResponderAuth so authentication becomes structural rather than dependent on user diligence.

Finding PCH-02-004: Crypto-adjacent dependencies are caret-ranged while core crypto deps are exact-pinned

Severity: Informational Location: package.json:31-52 Description: The core vault/crypto/transport dependencies are exact-pinned (no caret): @stablelib/* 2.0.1, hash-wasm 4.12.0, @bokuweb/zstd-wasm 0.0.27, @subspace/reed-solomon-erasure.wasm 0.2.5, tldts 7.0.28, pdfjs-dist 5.7.284, @zxcvbn-ts/* (this is the LV-9 exact-pin discipline). Several dependencies on or adjacent to a security path remain caret-ranged: otpauth ^9.5.0 (TOTP generation), pako ^2.1.0 (Codex decompression on the import side), @zxing/library ^0.23.0 and bwip-js ^4.10.1 (Aztec decode/encode), pdf-lib ^1.17.1, papaparse ^5.5.3. Notes: With a committed lockfile (bun.lock, packageManager pinned to [email protected], frozen-lockfile CI per LV-6) the practical exposure is limited to fresh resolves, but extending the exact-pin discipline to otpauth and pako in particular would make the supply-chain posture uniform with the rest of the crypto surface.

Finding PCH-02-005: Legacy encrypted-JSON import uses PBKDF2-SHA256 rather than Argon2id

Severity: Informational Location: src/shared/backup.ts:146-173 Description: The legacy .json export/import envelope derives its key with PBKDF2-SHA256 at 250,000 iterations, whereas every current path uses Argon2id (m=64 MiB, t=3, p=4). PBKDF2 with no memory-hardness is materially weaker against GPU/ASIC attackers than Argon2id at equivalent wall-clock cost. Notes: Scope is bounded and the decision is reasonable: this format's UI export was removed in v2.5.0 (backup.ts:5-12), the helper is import-only (plus a test fixture generator), and current exports use PeachScript / the QR matrix encoder backed by Argon2id. No action is strictly required; flagged so the KDF asymmetry is visible to anyone who still relies on old .json backups.

Finding PCH-02-006: vault_id is a stable, LAN-broadcast, cross-device-linkable identifier

Severity: Informational Location: src/shared/sync-identity.ts:62-75 Description: computeVaultId returns BLAKE3(masterKey || "peach-vault-id"), a deterministic 64-hex value that, per its own comment, is carried in the mDNS TXT record and the first sync handshake (SYNC_HELLO). It is therefore a stable identifier visible to any device on the same LAN, and it is identical across all of a user's paired devices, making them linkable to a passive local observer for as long as the master key (root key) is unchanged. Notes: This is inherent to the discovery design — peers must recognise "same vault" before exchanging entries — and it is one-way (the master key is not recoverable from it). It is a minor privacy property worth stating, not a confidentiality flaw. "Rotate sync identity" (vault.ts:2771-2814) changes it by re-keying the root key.

Finding PCH-02-007: HIBP breach check omits the Add-Padding response-padding header

Severity: Informational Location: src/background/vault.ts:216-238 Description: checkBreach queries the Have I Been Pwned k-anonymity range API with the SHA-1 prefix but does not send Add-Padding: true. HIBP supports that header to pad the response to a fixed bucket count; without it, the number of suffixes returned for a given prefix is a (weak) side channel that a network-position observer could use to narrow which prefix was queried. Notes: Impact is small — only the 5-hex-char prefix is ever sent, never the password or full hash, and breach monitoring is an opt-out convenience feature. Adding the padding header is a one-line hardening that costs nothing.

Finding PCH-02-008: peachPayload BinaryReader performs no explicit bounds checking

Severity: Informational Location: src/crypto/peachPayload.ts:940-969 (BinaryReader), consumed in decodeEntry 638-813 Description: The binary entry decoder reads u8/u16/u32/length-prefixed strings without validating that the cursor stays within the buffer. Past-end reads yield undefined/NaN (for u8) or throw RangeError (DataView), and length-prefixed reads clamp via subarray. The decoded bytes are normally inside an AES-GCM-authenticated envelope, but the same decoder runs on imported .peach payloads where an attacker who also knows the passphrase controls the plaintext. Notes: This is a robustness observation, not a memory-safety issue — the runtime is JavaScript, so the worst case is a thrown exception (caught by the import UI) or a malformed entry that is later rejected by sanitizeRestoredEntry at the vault boundary. The declared section-length and cumulative-size caps (peachPayload.ts:301-311, MV-4) already bound total work before this point. Explicit cursor bounds checks would make the decoder's failure modes uniform and self-evidently safe.

Finding PCH-02-009: X25519 shared-secret computations do not reject low-order/zero outputs

Severity: Informational Location: src/shared/noise-xx.ts:108,119,140,196,204,231, src/shared/noise-js.ts:84,106,175,248,263,362 Description: All sharedKey(secret, peerPublic) calls use @stablelib/x25519 with the default rejectZeroSharedKey = false, so a peer public key chosen as a low-order point (yielding an all-zero shared secret) is not rejected at the DH step. Notes: The practical impact is neutralised by the surrounding protocol: NK pre-mixes the known responder static key, the XX responder static is pinned against the paired-device registry before and after the handshake (chirp-sync-transport.ts:130-154), and pairing is additionally gated by the human SAS comparison. An attacker forcing a zero shared secret would still fail the static-key pin and/or the SAS. Passing rejectZero = true would make the DH contributory-by-construction and remove the need to reason about this at all.

Finding PCH-02-010: Clipboard auto-clear is unconditional and does not compare against the parked hash

Severity: Informational Location: src/background/clipboard.ts:98-117,124-178 Description: On copy, a SHA-256 of the copied value is parked in storage.session purely as a "a clear is owed" flag (98-103); the auto-clear routine (clearClipboardNow) then writes an empty string to the clipboard unconditionally when the timer/alarm fires, without reading the current clipboard to confirm it still holds the Peach-copied value. If the user copies something else within the clear window, that later value is wiped too. Notes: This is a deliberate trade-off in favour of the secret-must-not-persist invariant (the alarm path is eviction-proof, HV-2, and clearing an already-changed clipboard is harmless to confidentiality), and a content/popup-side write with a real user gesture is preferred when available (alreadyWritten). It is a minor UX surprise rather than a security issue; noted for completeness because the parked hash is computed but never used for a match check, which could read as dead bookkeeping to a future maintainer.

Positive Observations

The codebase's strengths are specific and worth recording at the same resolution as the findings.

Summary Table

ID Title Severity Location
PCH-02-001 Update-entry patch path bypasses canary/tags/passkey sanitizers Low vault.ts:2317-2417, messages.ts:478-501
PCH-02-002 Noise XX message 1 omits canonical empty-payload hash step Informational noise-xx.ts:93-97,184-191
PCH-02-003 Pairing auth rests on user SAS + unverifiable peer-side gate Informational chirp-handshake.ts:125-216
PCH-02-004 Crypto-adjacent deps caret-ranged vs. exact-pinned core Informational package.json:31-52
PCH-02-005 Legacy JSON import uses PBKDF2 rather than Argon2id Informational backup.ts:146-173
PCH-02-006 vault_id is stable, LAN-broadcast, cross-device-linkable Informational sync-identity.ts:62-75
PCH-02-007 HIBP check omits Add-Padding header Informational vault.ts:216-238
PCH-02-008 peachPayload BinaryReader lacks explicit bounds checks Informational peachPayload.ts:940-969
PCH-02-009 X25519 DH does not reject low-order/zero outputs Informational noise-xx.ts, noise-js.ts (DH call sites)
PCH-02-010 Clipboard auto-clear unconditional; parked hash unused for match Informational clipboard.ts:98-178

Conclusions

Peach is a carefully engineered, security-conscious password manager whose cryptographic core is standard and correctly parameterised, and whose extension trust boundaries are enforced with browser-attested origins, closed shadow-DOM UI, sender-gated destructive operations, and consistent secret minimisation across the content-script boundary. The vault layer handles the hard cases — atomic key-rotation writes, in-memory rollback on failed persists, monotonic timestamps, constant-time duress unlock — with evident care, and the higher-risk feature surfaces (LAN sync, Codex PDF import) are bounded against malformed and malicious input at multiple layers.

The review found no Critical or High issues. The single Low finding is a sanitizer asymmetry on the update path that is meaningfully mitigated by the extension's messaging model and is a local fix. The remaining items are Informational: documented residual risks (the pairing SAS dependency on an unverifiable peer-side gate is the most important to track, because its weakest link lives in a repository this review cannot see), small defense-in-depth hardening opportunities (reject zero DH outputs, add HIBP padding, bound the binary reader explicitly), and supply-chain/KDF consistency notes.

A technically literate reader should come away with high confidence in the correctness of the encryption-at-rest and autofill-boundary implementations, and with a short, concrete list of hardening steps rather than any structural concern. The pervasive, accurate in-code documentation of why each control exists is itself a quality signal: it makes the security model auditable from the source alone, which is exactly what a transparency-focused publication of this review should want to demonstrate. The two cross-repository dependencies — the Android Noise hand-port and the Android SAS gate — are the parts of the trust model that this review could not close from the browser source, and they are the right place to direct any follow-up verification.