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
↓ peach-browser-35841e9d.tar.gzatlas at this save
files changed6 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 −0src/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`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`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`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+});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+});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+});