35841e9da617

add PCH-02-007/008/009 fix drafts: HIBP Add-Padding header with padded-response parser proof + 3 tests, BinaryReader ensure() bounds on every read with PAYLOAD_TOO_SHORT + export seam + 8 tests, stablelib rejectZero=true at all 12 Noise DH sites + 5 tests

time
2026-06-12 20:14 UTC · 4 months ago
machine
koh2
agent
claude-fable-5
session
pch-02 fix drafts
parent
9bd1e7c2cf37
6 files src/drafts/PCH-02-FIXES/PCH-02-007-hibp-add-padding.patch.md+45 −0src/drafts/PCH-02-FIXES/PCH-02-008-binaryreader-bounds.patch.md+123 −0src/drafts/PCH-02-FIXES/PCH-02-009-reject-zero-dh.patch.md+78 −0src/drafts/PCH-02-FIXES/noise.reject-zero-dh.test.ts+113 −0src/drafts/PCH-02-FIXES/peachPayload.binaryreader-bounds.test.ts+74 −0src/drafts/PCH-02-FIXES/vault.hibp-padding.test.ts+85 −0
src/drafts/PCH-02-FIXES/PCH-02-007-hibp-add-padding.patch.md+45 −0
@@ -0,0 +1,45 @@1+# PCH-02-007 — Add `Add-Padding: true` to the HIBP k-anonymity range check2+3+**Severity:** Informational4+**Target file:** `src/background/vault.ts:230` (`checkBreach`)5+**Report:** peach-code-review.md, Finding PCH-02-0076+7+## Exact edit8+9+```typescript10+// BEFORE (vault.ts:230)11+    const res = await fetch(`https://api.pwnedpasswords.com/range/${prefix}`);12+13+// AFTER14+    // PCH-02-007: HIBP pads the response to a fixed bucket size when asked,15+    // so a network-position observer can't use response length to infer how16+    // many suffixes a prefix returned. Padding lines are random suffixes with17+    // count 0 — the suffix match below either misses them or parses 0, which18+    // every consumer already treats as "not breached".19+    const res = await fetch(`https://api.pwnedpasswords.com/range/${prefix}`, {20+      headers: { "Add-Padding": "true" },21+    });22+```23+24+## Parser compatibility (verified)25+26+Padded responses append fake `SUFFIX:0` lines. `checkBreach` parses27+`text.split("\r\n").find((l) => l.startsWith(suffix))` then28+`parseInt(line.split(":")[1], 10)`:29+30+- A padding line virtually never collides with the real 35-hex-char suffix;31+  if one ever did, its count parses to `0`, and both `checkBreach`'s `match ?`32+  expression and every caller (`breach > 0` / `count > 0` checks in33+  vault.ts/messages.ts) already treat 0 as not-breached.34+- No change needed to the response handling.35+36+## Tests37+38+`vault.hibp-padding.test.ts` (in this folder) — 3 tests: header present on the39+range request (fetch spy), breach count still parsed from a padded response,40+padding-line count-0 is not reported as a breach.41+Destination: `src/background/vault.hibp-padding.test.ts`.42+43+## Save message (integration)44+45+`fix: PCH-02-007 add Add-Padding header to HIBP k-anonymity range check`

view file

src/drafts/PCH-02-FIXES/PCH-02-008-binaryreader-bounds.patch.md+123 −0
@@ -0,0 +1,123 @@1+# PCH-02-008 — BinaryReader explicit bounds checks2+3+**Severity:** Informational4+**Target file:** `src/crypto/peachPayload.ts:940-969` (`BinaryReader`)5+**Report:** peach-code-review.md, Finding PCH-02-0086+7+## Current failure modes (verified by reading the class)8+9+- `u8()` past end → `undefined` (then `NaN` arithmetic downstream)10+- `u16()`/`u32()` past end → `RangeError` from DataView (engine-worded, not a11+  Peach error code)12+- `read(n)` past end → silently clamped short `subarray`13+- `lpString()` → short string from clamped `read`14+15+The decoded bytes normally sit inside a GCM-authenticated envelope, but the16+same decoder runs on imported `.peach` payloads where an attacker who knows17+the passphrase controls the plaintext (report text). The MV-4 declared-length18+and cumulative caps (peachPayload.ts:301–311) bound total work *before* this19+point; this fix makes the per-read failure mode uniform and self-evidently20+safe.21+22+## Error-code choice (deviation from the brief's sample text)23+24+The brief's sample throws prose messages ("BinaryReader: unexpected end of25+buffer reading u8"). This file's decode layer throws ALL_CAPS error codes26+(`PAYLOAD_TOO_SHORT`, `SECTION_LENGTH_MISMATCH`, `BAD_MAGIC`,27+`UNKNOWN_ENTRY_TYPE:*`) that the import UI maps to user copy. A truncated28+buffer mid-entry IS a too-short payload, so the bounds check throws the29+existing **`PAYLOAD_TOO_SHORT`** code — the import UI handles it today with30+zero UI changes. (Distinct prose codes would surface as the generic error31+string instead.)32+33+## Exact replacement — BinaryReader class34+35+```typescript36+// REPLACE the whole class (peachPayload.ts:940-969) with:37+38+class BinaryReader {39+  cursor: number;40+  private view: DataView;41+  constructor(private data: Uint8Array, start: number) {42+    this.cursor = start;43+    this.view = new DataView(data.buffer, data.byteOffset, data.byteLength);44+  }45+  // PCH-02-008: every read validates the cursor stays inside the buffer. The46+  // payload normally arrives through a GCM-authenticated envelope, but the47+  // same decoder runs on imported `.peach` files where the plaintext is48+  // attacker-shaped (given the passphrase); a section truncated mid-entry49+  // must fail with the same PAYLOAD_TOO_SHORT code the section parser uses,50+  // never with undefined/NaN reads or engine RangeErrors.51+  private ensure(n: number): void {52+    if (this.cursor + n > this.data.length) throw new Error("PAYLOAD_TOO_SHORT");53+  }54+  u8(): number {55+    this.ensure(1);56+    return this.data[this.cursor++];57+  }58+  u16(): number {59+    this.ensure(2);60+    const v = this.view.getUint16(this.cursor, true);61+    this.cursor += 2;62+    return v;63+  }64+  u32(): number {65+    this.ensure(4);66+    const v = this.view.getUint32(this.cursor, true);67+    this.cursor += 4;68+    return v;69+  }70+  read(n: number): Uint8Array {71+    this.ensure(n);72+    const out = this.data.subarray(this.cursor, this.cursor + n);73+    this.cursor += n;74+    return out;75+  }76+  lpString(lenSize: 1 | 2): string {77+    const len = lenSize === 1 ? this.u8() : this.u16();78+    return new TextDecoder().decode(this.read(len));79+  }80+}81+```82+83+Notes:84+- `ensure` uses plain number addition; the largest `n` is a `u32` blob length85+  (≤ 2³²−1), far inside float53 — no overflow hazard.86+- `view` construction is unchanged: it is already bounded to the87+  `data` subarray (`byteOffset`/`byteLength`), so a reader can never see88+  sibling bytes of a shared ArrayBuffer.89+- `decodeEntry`'s `cursor: reader.cursor` return contract is unchanged.90+91+## Test seam — export the class92+93+`BinaryReader` is module-private and only reachable end-to-end through a full94+encrypted envelope, which cannot express "truncated mid-entry" without also95+failing the section-length check first. Export it for direct regression96+coverage (same precedent as the `initHash`/`hkdf2` exports added for the97+PCH-01-002 KATs):98+99+```typescript100+// BEFORE (peachPayload.ts:940)101+class BinaryReader {102+103+// AFTER104+// Exported for the PCH-02-008 bounds-check regression tests only — not part105+// of the payload API surface.106+export class BinaryReader {107+```108+109+## Tests110+111+`peachPayload.binaryreader-bounds.test.ts` (in this folder) — 8 tests: each112+read method past-end throws `PAYLOAD_TOO_SHORT` (u8/u16/u32/read/lpString,113+including a declared length-prefix that overruns), valid reads at exact114+boundaries still succeed, cursor advances correctly (regression), and a115+non-zero `start` offset is respected.116+Destination: `src/crypto/peachPayload.binaryreader-bounds.test.ts`.117+118+Existing round-trip coverage (`peachPayload.test.ts`, `peachPayload.bomb.test.ts`)119+regression-guards the happy decode path end-to-end.120+121+## Save message (integration)122+123+`fix: PCH-02-008 BinaryReader explicit bounds checks on every read — uniform PAYLOAD_TOO_SHORT on truncation`

view file

src/drafts/PCH-02-FIXES/PCH-02-009-reject-zero-dh.patch.md+78 −0
@@ -0,0 +1,78 @@1+# PCH-02-009 — Reject zero X25519 DH outputs in Noise XX and NK2+3+**Severity:** Informational4+**Target files:** `src/shared/noise-xx.ts` (6 DH sites),5+`src/shared/noise-js.ts` (6 DH sites)6+**Report:** peach-code-review.md, Finding PCH-02-0097+8+## Implementation choice (simpler than the brief's sample)9+10+`@stablelib/x25519`'s `sharedKey` already has the exact guard built in:11+`sharedKey(mySecretKey, theirPublicKey, rejectZero = false)` — with12+`rejectZero: true` it throws `"X25519: invalid shared key"` on an all-zero13+output (verified in `node_modules/@stablelib/x25519/lib/x25519.js:606-624`).14+Passing `true` at every call site is equivalent to the brief's manual15+`every(b => b === 0)` check, applies to the raw 32-byte output before it16+reaches `mixKey`/HMAC, and adds no new code path to maintain.17+18+**Not a wire change.** Honest peers never produce an all-zero shared secret19+(the probability for random keys is negligible; only adversarially chosen20+low-order/identity points do). Each side can adopt the check independently —21+no Android coordination required, though mirroring it there is recommended.22+23+## Exact edits — all 12 DH call sites24+25+`noise-xx.ts` — add a rationale comment at the first site, `, true` at all six:26+27+```typescript28+// Line 108 (initiator, ee):29+// BEFORE30+  const ee = sharedKey(e.secretKey, remoteEphemeral);31+// AFTER32+  // PCH-02-009: rejectZero — an all-zero DH output (low-order peer point)33+  // aborts the handshake before the value can enter the key schedule. Applies34+  // at every DH below; honest peers are unaffected.35+  const ee = sharedKey(e.secretKey, remoteEphemeral, true);36+37+// Line 119 (initiator, es):   sharedKey(e.secretKey, remoteStaticPlain, true)38+// Line 140 (initiator, se):   sharedKey(ourStaticPrivateKey, remoteEphemeral, true)39+// Line 196 (responder, ee):   sharedKey(e.secretKey, remoteEphemeral, true)40+// Line 204 (responder, es):   sharedKey(ourStaticPrivateKey, remoteEphemeral, true)41+// Line 231 (responder, se):   sharedKey(e.secretKey, remoteStaticPlain, true)42+```43+44+`noise-js.ts` — same pattern, comment at the first site:45+46+```typescript47+// Line 84  (NK initiator, es):  sharedKey(ephemeral.secretKey, responderStaticPublicKey, true)48+//   with the PCH-02-009 rationale comment above it (as in noise-xx.ts)49+// Line 106 (NK initiator, ee):  sharedKey(ephemeral.secretKey, remoteEphemeralPublic, true)50+// Line 175 (computeAuthProof):  sharedKey(initiatorPrivate, responderPublic, true)51+// Line 248 (NK responder, es):  sharedKey(ourStaticPrivateKey, remoteEphemeralPublic, true)52+// Line 263 (NK responder, ee):  sharedKey(ephemeral.secretKey, remoteEphemeralPublic, true)53+// Line 362 (performResponderAuth): sharedKey(ourPrivateKey, initiatorPublicKey, true)54+```55+56+The two auth-proof sites (175, 362) key an HMAC rather than `mixKey`; the57+same contributory-behaviour argument applies — a zero MAC key derived from an58+attacker-chosen point should abort, not authenticate.59+60+## KAT / existing-test impact61+62+None. The pinned vectors (`noise-js.test.ts` self-vectors,63+`noise-blake2s-kat.test.ts` cross-repo vectors) all use honest keys whose DH64+outputs are non-zero; `rejectZero` only changes behaviour on the degenerate65+inputs the new tests inject.66+67+## Tests68+69+`noise.reject-zero-dh.test.ts` (in this folder) — 5 tests: NK initiator70+aborts against an all-zero (low-order) responder static; NK responder aborts71+on an all-zero msg-1 ephemeral; XX responder aborts on an all-zero msg-172+ephemeral; full NK and XX handshakes over an in-memory transport still73+succeed with honest keys (regression).74+Destination: `src/shared/noise.reject-zero-dh.test.ts`.75+76+## Save message (integration)77+78+`fix: PCH-02-009 reject zero X25519 DH output at all 12 Noise XX/NK call sites via stablelib rejectZero`

view file

src/drafts/PCH-02-FIXES/noise.reject-zero-dh.test.ts+113 −0
@@ -0,0 +1,113 @@1+// PCH-02-009 regression tests — every Noise XX/NK DH must reject an all-zero2+// shared secret (low-order / identity peer point) instead of feeding it into3+// the key schedule. Relies on `sharedKey(…, …, true)` from the PCH-02-0094+// patch, which throws "X25519: invalid shared key".5+//6+// DESTINATION: src/shared/noise.reject-zero-dh.test.ts7+// Imports are written in destination form; this file is excluded from the8+// suite while it lives under src/drafts/.9+10+import { describe, expect, it } from "vitest";11+import { generateKeyPair } from "@stablelib/x25519";12+import { noiseNKInitiator, noiseNKResponder } from "./noise-js";13+import { noiseXXInitiator, noiseXXResponder } from "./noise-xx";14+15+// The identity element: X25519 with an all-zero public key yields an all-zero16+// shared secret — the canonical low-order input the fix must reject.17+const ZERO_POINT = new Uint8Array(32);18+19+function queuePair() {20+  const makeQueue = () => {21+    const items: Uint8Array[] = [];22+    const waiters: Array<(m: Uint8Array) => void> = [];23+    return {24+      push(m: Uint8Array) {25+        const w = waiters.shift();26+        if (w) w(m);27+        else items.push(m);28+      },29+      pull(): Promise<Uint8Array> {30+        const m = items.shift();31+        return m ? Promise.resolve(m) : new Promise((r) => waiters.push(r));32+      },33+    };34+  };35+  const aToB = makeQueue();36+  const bToA = makeQueue();37+  return {38+    a: { send: async (m: Uint8Array) => aToB.push(m), receive: () => bToA.pull() },39+    b: { send: async (m: Uint8Array) => bToA.push(m), receive: () => aToB.pull() },40+  };41+}42+43+const deadTransport = {44+  send: async () => undefined,45+  receive: () => new Promise<Uint8Array>(() => undefined), // never resolves46+};47+48+describe("PCH-02-009 — reject zero X25519 DH outputs", () => {49+  it("NK initiator aborts against an all-zero responder static key", async () => {50+    // es = DH(ephemeral, ZERO_POINT) is computed before msg1 is sent, so the51+    // handshake must die before anything touches the transport.52+    await expect(53+      noiseNKInitiator(ZERO_POINT, deadTransport.send, deadTransport.receive),54+    ).rejects.toThrow(/invalid shared key/);55+  });56+57+  it("NK responder aborts on an all-zero msg-1 ephemeral", async () => {58+    const ourStatic = generateKeyPair();59+    // 32 zero bytes of "ephemeral" + 16 bytes of fake AEAD tag = minimum frame.60+    const msg1 = new Uint8Array(48);61+    await expect(62+      noiseNKResponder(63+        ourStatic.secretKey,64+        ourStatic.publicKey,65+        deadTransport.send,66+        async () => msg1,67+      ),68+    ).rejects.toThrow(/invalid shared key/);69+  });70+71+  it("XX responder aborts on an all-zero msg-1 ephemeral", async () => {72+    const ourStatic = generateKeyPair();73+    await expect(74+      noiseXXResponder(75+        ourStatic.secretKey,76+        ourStatic.publicKey,77+        deadTransport.send,78+        async () => ZERO_POINT.slice(),79+      ),80+    ).rejects.toThrow(/invalid shared key/);81+  });82+83+  it("honest NK handshake still completes and transports a frame (regression)", async () => {84+    const responderStatic = generateKeyPair();85+    const { a, b } = queuePair();86+87+    const [initiator, responder] = await Promise.all([88+      noiseNKInitiator(responderStatic.publicKey, a.send, a.receive),89+      noiseNKResponder(responderStatic.secretKey, responderStatic.publicKey, b.send, b.receive),90+    ]);91+92+    const frame = new TextEncoder().encode("peach-nk-frame");93+    expect(responder.decrypt(initiator.encrypt(frame))).toEqual(frame);94+    expect(initiator.handshakeHash).toEqual(responder.handshakeHash);95+  });96+97+  it("honest XX handshake still completes and transports a frame (regression)", async () => {98+    const initiatorStatic = generateKeyPair();99+    const responderStatic = generateKeyPair();100+    const { a, b } = queuePair();101+102+    const [initiator, responder] = await Promise.all([103+      noiseXXInitiator(initiatorStatic.secretKey, initiatorStatic.publicKey, a.send, a.receive),104+      noiseXXResponder(responderStatic.secretKey, responderStatic.publicKey, b.send, b.receive),105+    ]);106+107+    expect(initiator.remoteStaticPublicKey).toEqual(responderStatic.publicKey);108+    expect(responder.remoteStaticPublicKey).toEqual(initiatorStatic.publicKey);109+110+    const frame = new TextEncoder().encode("peach-xx-frame");111+    expect(responder.decrypt(initiator.encrypt(frame))).toEqual(frame);112+  });113+});

view file

src/drafts/PCH-02-FIXES/peachPayload.binaryreader-bounds.test.ts+74 −0
@@ -0,0 +1,74 @@1+// PCH-02-008 regression tests — BinaryReader must bound every read and fail2+// with the decoder's existing PAYLOAD_TOO_SHORT code on truncated input.3+//4+// DESTINATION: src/crypto/peachPayload.binaryreader-bounds.test.ts5+// Requires the `export class BinaryReader` seam from the PCH-02-008 patch.6+// Imports are written in destination form; this file is excluded from the7+// suite while it lives under src/drafts/.8+9+import { describe, expect, it } from "vitest";10+import { BinaryReader } from "./peachPayload";11+12+describe("PCH-02-008 — BinaryReader bounds checks", () => {13+  it("u8 past end of buffer throws PAYLOAD_TOO_SHORT", () => {14+    const r = new BinaryReader(Uint8Array.of(0xaa), 0);15+    expect(r.u8()).toBe(0xaa);16+    expect(() => r.u8()).toThrow("PAYLOAD_TOO_SHORT");17+  });18+19+  it("u16 with only one byte remaining throws PAYLOAD_TOO_SHORT", () => {20+    const r = new BinaryReader(Uint8Array.of(0x01), 0);21+    expect(() => r.u16()).toThrow("PAYLOAD_TOO_SHORT");22+  });23+24+  it("u32 with three bytes remaining throws PAYLOAD_TOO_SHORT", () => {25+    const r = new BinaryReader(Uint8Array.of(0x01, 0x02, 0x03), 0);26+    expect(() => r.u32()).toThrow("PAYLOAD_TOO_SHORT");27+  });28+29+  it("read(n) past end throws instead of clamping short", () => {30+    const r = new BinaryReader(Uint8Array.of(1, 2, 3, 4), 0);31+    expect(() => r.read(5)).toThrow("PAYLOAD_TOO_SHORT");32+  });33+34+  it("lpString whose declared length overruns the buffer throws", () => {35+    // length prefix says 200 bytes; only 2 follow.36+    const r = new BinaryReader(Uint8Array.of(200, 0x61, 0x62), 0);37+    expect(() => r.lpString(1)).toThrow("PAYLOAD_TOO_SHORT");38+  });39+40+  it("valid reads at exact buffer boundaries still work (regression)", () => {41+    const buf = new Uint8Array(11);42+    const view = new DataView(buf.buffer);43+    buf[0] = 0x7f;44+    view.setUint16(1, 0xbeef, true);45+    view.setUint32(3, 0xdeadbeef, true);46+    buf.set([0x70, 0x65, 0x61, 0x63], 7); // "peac"47+48+    const r = new BinaryReader(buf, 0);49+    expect(r.u8()).toBe(0x7f);50+    expect(r.u16()).toBe(0xbeef);51+    expect(r.u32()).toBe(0xdeadbeef);52+    expect(Array.from(r.read(4))).toEqual([0x70, 0x65, 0x61, 0x63]);53+    // Buffer fully consumed; the very next read must throw.54+    expect(() => r.u8()).toThrow("PAYLOAD_TOO_SHORT");55+  });56+57+  it("lpString round-trips a valid length-prefixed string (regression)", () => {58+    const text = new TextEncoder().encode("peach");59+    const buf = new Uint8Array(1 + text.length);60+    buf[0] = text.length;61+    buf.set(text, 1);62+63+    const r = new BinaryReader(buf, 0);64+    expect(r.lpString(1)).toBe("peach");65+    expect(r.cursor).toBe(buf.length);66+  });67+68+  it("respects a non-zero start offset and bounds from it", () => {69+    const buf = Uint8Array.of(0xff, 0xff, 0x2a);70+    const r = new BinaryReader(buf, 2);71+    expect(r.u8()).toBe(0x2a);72+    expect(() => r.u8()).toThrow("PAYLOAD_TOO_SHORT");73+  });74+});

view file

src/drafts/PCH-02-FIXES/vault.hibp-padding.test.ts+85 −0
@@ -0,0 +1,85 @@1+// PCH-02-007 regression tests — the HIBP k-anonymity range request must ask2+// for response padding, and the parser must stay correct on padded responses.3+//4+// DESTINATION: src/background/vault.hibp-padding.test.ts5+// Imports are written in destination form; this file is excluded from the6+// suite while it lives under src/drafts/.7+8+import { beforeEach, describe, expect, it, vi } from "vitest";9+10+// SHA-1("password") = 5BAA61E4C9B93F3F0682250B6CF8331B7EE68FD811+const PREFIX = "5BAA6";12+const SUFFIX = "1E4C9B93F3F0682250B6CF8331B7EE68FD8";13+14+describe("PCH-02-007 — HIBP Add-Padding", () => {15+  let fetchMock: ReturnType<typeof vi.fn>;16+17+  beforeEach(() => {18+    vi.resetModules();19+20+    const chromeMock = {21+      storage: {22+        local: {23+          get: vi.fn(async () => ({})),24+          set: vi.fn(async () => undefined),25+          remove: vi.fn(async () => undefined),26+        },27+      },28+      alarms: {29+        clear: vi.fn(),30+        create: vi.fn(),31+        onAlarm: { addListener: vi.fn() },32+      },33+      idle: {34+        setDetectionInterval: vi.fn(),35+        onStateChanged: { addListener: vi.fn() },36+      },37+    } as unknown as typeof chrome;38+    (globalThis as { chrome?: typeof chrome }).chrome = chromeMock;39+40+    fetchMock = vi.fn(async () => ({41+      ok: true,42+      text: async () =>43+        [44+          "0018A45C4D1DEF81644B54AB7F969B88D65:1",45+          `${SUFFIX}:3303003`,46+          // Padding lines HIBP appends under Add-Padding — always count 0.47+          "FFFFFFAAAAAABBBBBBCCCCCCDDDDDDEEEEE:0",48+        ].join("\r\n"),49+    }));50+    vi.stubGlobal("fetch", fetchMock);51+  });52+53+  it("sends Add-Padding: true on the range request", async () => {54+    const vault = await import("./vault");55+    await vault.checkBreach("password");56+57+    expect(fetchMock).toHaveBeenCalledWith(58+      `https://api.pwnedpasswords.com/range/${PREFIX}`,59+      expect.objectContaining({60+        headers: expect.objectContaining({ "Add-Padding": "true" }),61+      }),62+    );63+  });64+65+  it("still parses the real breach count out of a padded response", async () => {66+    const vault = await import("./vault");67+    const count = await vault.checkBreach("password");68+    expect(count).toBe(3303003);69+  });70+71+  it("reports 0 when the response holds only padding lines", async () => {72+    fetchMock.mockImplementationOnce(async () => ({73+      ok: true,74+      text: async () =>75+        [76+          "FFFFFFAAAAAABBBBBBCCCCCCDDDDDDEEEEE:0",77+          "EEEEEEAAAAAABBBBBBCCCCCCDDDDDDFFFFF:0",78+        ].join("\r\n"),79+    }));80+81+    const vault = await import("./vault");82+    const count = await vault.checkBreach("password");83+    expect(count).toBe(0);84+  });85+});

view file