From a42a3467426f1384fa72da55819440c32c2ab901 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Wed, 5 Aug 2026 19:56:01 +0200 Subject: [PATCH] fix(security): a link cannot take over the app, and a stray frame cannot kill it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from an adversarial audit, both confirmed by independent verifiers, both reproduced as failing tests before being fixed. A pairing link paired the app by itself. Pair.svelte ran the fragment the moment it arrived, with no screen and no tap. A link is something anyone can send — by SMS, by email, on a sticker over the real QR — so "your box needs re-pairing, tap here" silently repointed the app at the sender's box: their readings shown as this home, every mode change sent to their hardware, and no way back without finding the physical code. On a device without PRF it cost the owner not one interaction. Worse, this repo made it easier earlier today: the leftover-fragment fix treats "a different box" as evidence of a genuine invitation, which is exactly what an attacker's link is. A link is an offer now. The box's key is shown as a six-character fingerprint, replacing an existing home is named as what it does, and nothing is trusted until someone agrees. Scanning with the camera is a deliberate act already, so that path still pairs on the spot. storeSite no longer moves localStorage['ftw.site'] as a side effect either: storing a site is a fact, making it the app's home is a decision, and only the caller who saw the user agree may take it. A stray frame killed the carrier permanently. Every inbound frame was fed to the handshake while awaiting message 2, and a read failure closed the carrier non-retryably. The relay broadcasts the box's frames to every stream in a room, so a second phone in the same house starts its handshake into a running 1 Hz telemetry stream and dies on the first frame — a household where one phone works and the second never can. It also handed anyone able to write to the socket a one-packet kill switch. Only message-2-shaped frames reach the handshake now, a frame that does not open is dropped rather than fatal, and a deadline ends a handshake that is truly going nowhere — retryably, which matters because a box refuses by staying silent, so silence had no ending at all. Co-Authored-By: Claude Opus 5 --- contract/registry.yaml | 3 + src/App.svelte | 5 + src/lib/carrier/handshake-resilience.test.ts | 106 +++++++++++++++++++ src/lib/carrier/noise.ts | 50 +++++++-- src/lib/identity/pairing.ts | 14 ++- src/views/Pair.svelte | 76 ++++++++++++- src/views/Pair.svelte.test.ts | 64 +++++++++++ 7 files changed, 307 insertions(+), 11 deletions(-) create mode 100644 src/lib/carrier/handshake-resilience.test.ts create mode 100644 src/views/Pair.svelte.test.ts diff --git a/contract/registry.yaml b/contract/registry.yaml index 072458a..1ef4d8f 100644 --- a/contract/registry.yaml +++ b/contract/registry.yaml @@ -51,6 +51,9 @@ capabilities: - der.v2x - plan.dispatch - net.webrtc + # Electricity prices, when the box has a zone configured and rows stored. + # Absent means the app draws no price view rather than an empty one. + - price.spot # --------------------------------------------------------------------------- # Scopes. One object axis, two verb axes: .. diff --git a/src/App.svelte b/src/App.svelte index 1fcf9dd..851cd73 100644 --- a/src/App.svelte +++ b/src/App.svelte @@ -179,6 +179,11 @@ } function onPaired(pairedSiteId: string) { + // The decision the user just made, recorded here rather than as a side + // effect of storing the row — see setCurrentSite. + void import('$lib/identity/pairing').then(({ setCurrentSite }) => + setCurrentSite(pairedSiteId) + ) siteId = pairedSiteId // The /p#… URL just did its one job. Left in place it becomes the URL // the browser reloads and restores — with a spent code — which is how diff --git a/src/lib/carrier/handshake-resilience.test.ts b/src/lib/carrier/handshake-resilience.test.ts new file mode 100644 index 0000000..3984d99 --- /dev/null +++ b/src/lib/carrier/handshake-resilience.test.ts @@ -0,0 +1,106 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { NoiseCarrier } from './noise' +import { CarrierBase, type Carrier, type CarrierStatus } from './carrier' +import { generateKeyPair } from '$lib/crypto/noise' +import type { CarrierState } from '$lib/protocol/types' + +/* A stray frame during the handshake must not end the carrier. + * + * The relay broadcasts the box's frames to every stream in a room, so a + * second phone in the same house starts its handshake into a running 1 Hz + * telemetry stream. Those frames are not message 2. Feeding them to + * readMessage used to close the carrier non-retryably — a household where + * the first phone works and the second never can, and a one-packet kill + * switch for anyone able to write to the socket. + */ + +/** An inner carrier a test can push arbitrary bytes through. */ +class FakeInner extends CarrierBase implements Carrier { + readonly kind: CarrierState = 'relay' + readonly sent: Uint8Array[] = [] + #status: CarrierStatus = { phase: 'connecting' } + + get rttMs(): number | null { + return null + } + get status(): CarrierStatus { + return this.#status + } + send(frame: Uint8Array): void { + this.sent.push(frame) + } + close(): void { + this.#status = { phase: 'closed', reason: 'test', retryable: true } + this.emitStatus(this.#status) + } + open(): void { + this.#status = { phase: 'open', sinceMs: Date.now() } + this.emitStatus(this.#status) + } + deliver(bytes: Uint8Array): void { + this.emitFrame(bytes) + } +} + +function carrierUnderTest() { + const inner = new FakeInner() + const app = generateKeyPair() + const box = generateKeyPair() + const carrier = new NoiseCarrier({ + inner, + staticKey: app, + remoteStatic: box.publicKey, + }) + const seen: CarrierStatus[] = [] + carrier.onStatus((s) => seen.push(s)) + return { inner, carrier, seen } +} + +describe('a handshake meeting somebody else’s frames', () => { + beforeEach(() => vi.useFakeTimers()) + afterEach(() => vi.useRealTimers()) + + it('ignores a telemetry-sized frame instead of dying on it', async () => { + const { inner, carrier, seen } = carrierUnderTest() + inner.open() + await vi.advanceTimersByTimeAsync(10) + + // A lane 0 frame from the other phone's session: right shape for the + // wire, wrong thing entirely for this handshake. + inner.deliver(new Uint8Array(512)) + inner.deliver(new Uint8Array(280)) + await vi.advanceTimersByTimeAsync(10) + + expect( + seen.some((s) => s.phase === 'closed'), + 'a broadcast frame closed the carrier' + ).toBe(false) + expect(carrier.status.phase).not.toBe('closed') + }) + + it('ignores a wrong-key reply of the right length and keeps waiting', async () => { + const { inner, carrier, seen } = carrierUnderTest() + inner.open() + await vi.advanceTimersByTimeAsync(10) + + // 48 bytes: exactly message 2's shape, but not from the pinned box. + inner.deliver(new Uint8Array(48).fill(9)) + await vi.advanceTimersByTimeAsync(10) + + expect(seen.some((s) => s.phase === 'closed')).toBe(false) + expect(carrier.status.phase).toBe('connecting') + }) + + it('gives up retryably when the box never answers at all', async () => { + // A box that refuses a handshake stays silent on purpose — a reply would + // confirm a box is on this handle. Without a deadline a revoked phone + // sits on "Reaching your box" forever, socket open, never reconnecting. + const { inner, seen } = carrierUnderTest() + inner.open() + await vi.advanceTimersByTimeAsync(15_000) + + const closed = seen.find((s) => s.phase === 'closed') + expect(closed, 'silence never ended the handshake').toBeDefined() + expect(closed && 'retryable' in closed && closed.retryable).toBe(true) + }) +}) diff --git a/src/lib/carrier/noise.ts b/src/lib/carrier/noise.ts index 3008200..abf4bc4 100644 --- a/src/lib/carrier/noise.ts +++ b/src/lib/carrier/noise.ts @@ -16,9 +16,24 @@ import { CarrierBase, type Carrier, type CarrierStatus } from './carrier' import type { CarrierState } from '$lib/protocol/types' -import { HandshakeState, NoiseError, type StaticKey, type KeyPair } from '$lib/crypto/noise' +import { DH_BYTES, TAG_BYTES, HandshakeState, NoiseError, type StaticKey, type KeyPair } from '$lib/crypto/noise' import { NoiseTransport } from '$lib/crypto/transport' +/** + * How long a handshake may go unanswered before it is retried. + * + * The box answers a refused handshake with silence rather than a rejection, + * so this is the only thing that distinguishes "not yet" from "never". + */ +const HANDSHAKE_DEADLINE_MS = 12_000 + +/** + * Message 2 of Noise_IK: the responder's ephemeral public key and one AEAD + * tag over an empty payload. Fixed by the pattern, so anything else on the + * wire is somebody else's frame. + */ +const MESSAGE_2_BYTES = DH_BYTES + TAG_BYTES + export interface NoiseCarrierOptions { /** The transport to wrap. Its lifetime becomes ours. */ inner: Carrier @@ -57,6 +72,8 @@ export class NoiseCarrier extends CarrierBase implements Carrier { #closed = false /** True once message 1 is out and we are waiting for the reply. */ #awaitingReply = false + #deadline: ReturnType | undefined + #log: ((line: string) => void) | undefined /** Kept so each reconnection can start a fresh handshake from the same input. */ #seed: { staticKey: StaticKey | KeyPair; remoteStatic: Uint8Array; prologue?: Uint8Array } @@ -153,6 +170,16 @@ export class NoiseCarrier extends CarrierBase implements Carrier { this.#awaitingReply = true this.#setStatus({ phase: 'connecting' }) + // A box that refuses a handshake answers with silence, on purpose: a + // reply would confirm a box is on this handle. So silence needs its own + // ending, or a revoked phone sits on "Reaching your box" forever with the + // socket wide open and no reconnect ever firing. + clearTimeout(this.#deadline) + this.#deadline = setTimeout(() => { + if (this.#closed || !this.#awaitingReply) return + this.#fail('the box did not answer', true) + }, HANDSHAKE_DEADLINE_MS) + this.#handshake .writeMessage(this.#payload) .then((msg) => this.#inner.send(msg)) @@ -163,7 +190,15 @@ export class NoiseCarrier extends CarrierBase implements Carrier { if (this.#closed) return if (this.#awaitingReply) { - this.#completeHandshake(bytes) + // Only something the right shape is offered to the handshake. + // + // The relay broadcasts the box's frames to every stream in the room, so + // a second phone in the same house starts its handshake into a running + // 1 Hz telemetry stream. Those frames are not message 2, and feeding + // them to readMessage used to kill the carrier outright — a household + // where the first phone works and the second never can. It also handed + // anyone who can write to the socket a one-packet kill switch. + if (bytes.length === MESSAGE_2_BYTES) this.#completeHandshake(bytes) return } @@ -191,10 +226,13 @@ export class NoiseCarrier extends CarrierBase implements Carrier { this.#setStatus({ phase: 'open', sinceMs: Date.now() }) }) .catch((err) => { - // The commonest cause is the pinned key not matching what answered — - // which is exactly the check working. Not retryable: dialling again - // would meet the same wrong peer. - this.#fail(err instanceof NoiseError ? err.message : 'handshake rejected', false) + // Still not fatal, even at the right length: on a shared room another + // phone's transport frame can match by coincidence. A frame that does + // not open is a frame addressed to somebody else — drop it and keep + // waiting. The deadline below is what ends a handshake that is truly + // going nowhere, and it ends it retryably. + if (this.#closed || this.#handshake !== handshake) return + this.#log?.(err instanceof NoiseError ? err.message : 'handshake frame ignored') }) } diff --git a/src/lib/identity/pairing.ts b/src/lib/identity/pairing.ts index 587b948..6c768e6 100644 --- a/src/lib/identity/pairing.ts +++ b/src/lib/identity/pairing.ts @@ -148,10 +148,22 @@ async function storeSite(site: PairedSite): Promise { } await database.put('sites', row) +} + +/** + * Point the app at a site. + * + * Separate from storing it, on purpose. Storing a site is a fact; making it + * the site the app shows and controls is a decision, and it used to happen as + * a side effect of the fact — so any link that got as far as storing a row + * also silently became this phone's home. The caller makes that decision + * after the user has agreed to it. + */ +export function setCurrentSite(siteId: string): void { // The inline boot script reads this before the bundle is parsed, which is // what lets a cold start paint cached readings in the first frame. try { - localStorage.setItem('ftw.site', site.siteId) + localStorage.setItem('ftw.site', siteId) } catch { // Blocked storage costs a slower start, not a broken pairing. } diff --git a/src/views/Pair.svelte b/src/views/Pair.svelte index 12b24dd..cf2a551 100644 --- a/src/views/Pair.svelte +++ b/src/views/Pair.svelte @@ -60,13 +60,52 @@ onDestroy(() => handle?.stop()) - // Arrived with a link: pair straight away. Nothing to scan, and asking the - // user to press a button before doing what they already asked for is one - // tap too many. + /** + * A link is an offer, never an instruction. + * + * This used to pair the moment a fragment arrived. A link is something + * anyone can send — by SMS, by email, on a sticker over the real QR — so + * "your box needs re-pairing, tap here" silently repointed the app at the + * sender's box: their readings shown as this home, every mode change sent + * to their hardware, and no way back without finding the physical code + * again. On a device without PRF it cost the owner not one tap. + * + * So the fragment is parsed and shown, and nothing is trusted until + * someone says so. Scanning a code with the camera is a deliberate act + * already, so that path still pairs on the spot. + */ + let offered = $state<{ fragment: string; fingerprint: string } | null>(null) + $effect(() => { - if (fragment && stage === 'intro') void pair(fragment) + if (!fragment || stage !== 'intro') return + void (async () => { + const { parseEnrollmentFragment } = await import('$lib/identity/enrollment') + try { + const enrollment = parseEnrollmentFragment(fragment) + offered = { fragment, fingerprint: await fingerprintOf(enrollment.boxStaticPublic) } + } catch { + stage = 'error' + message = 'That link is not an FTW pairing code.' + } + })() }) + /** + * A short, stable name for a box key. + * + * Six hex characters of its digest. Not a security control on its own — + * nobody memorises it — but it makes two different boxes visibly + * different, which is what a person needs to notice that the box being + * offered is not the one on their wall. + */ + async function fingerprintOf(key: Uint8Array): Promise { + const digest = await crypto.subtle.digest('SHA-256', key as BufferSource) + return Array.from(new Uint8Array(digest).subarray(0, 3)) + .map((b) => b.toString(16).padStart(2, '0')) + .join('') + .toUpperCase() + } + async function startScan() { stage = 'scanning' message = '' @@ -125,6 +164,35 @@ {:else if stage === 'pairing'}

Connecting

Confirming it's really your box.

+ {:else if offered} + +

{known ? 'Connect to a different box?' : 'Connect this box?'}

+

+ This link points at box {offered.fingerprint}. + {#if known} + Connecting it replaces {known.label} as the home this app shows and + controls. Your key for {known.label} stays on this phone. + {:else} + Check it matches the code on your box before continuing. + {/if} +

+ + {#if message} +

{message}

+ {/if} + + + {:else}

{known ? 'Welcome back' : 'Connect your box'}

diff --git a/src/views/Pair.svelte.test.ts b/src/views/Pair.svelte.test.ts new file mode 100644 index 0000000..ce40cd2 --- /dev/null +++ b/src/views/Pair.svelte.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect, vi } from 'vitest' +import 'fake-indexeddb/auto' +import { render, screen } from '@testing-library/svelte' +import Pair from './Pair.svelte' +import { buildEnrollmentUrl } from '$lib/identity/enrollment' + +/* A pairing link must never pair by itself. + * + * A link is something anyone can send — by SMS, by email, on a sticker over + * the real QR. This screen used to pair the moment a fragment arrived, so + * "your box needs re-pairing, tap here" silently repointed the app at the + * sender's box: their readings shown as this home, every mode change sent to + * their hardware, and no way back without the physical code. On a device + * without PRF it cost the owner not one tap. + */ + +const ATTACKER_KEY = new Uint8Array(32).fill(0xbb) + +function fragmentFor(key: Uint8Array): string { + const url = buildEnrollmentUrl({ + boxStaticPublic: key, + pairingCode: new Uint8Array(16).fill(1), + rendezvousSecret: new Uint8Array(32).fill(2), + lanHint: '', + }) + return '#' + url.split('#')[1] +} + +describe('a pairing link that arrives on its own', () => { + it('is shown as an offer and pairs nothing until someone agrees', async () => { + const onPaired = vi.fn() + render(Pair, { props: { fragment: fragmentFor(ATTACKER_KEY), onPaired } }) + + // The decisive assertion: nothing was paired by the mere arrival of a link. + await new Promise((r) => setTimeout(r, 50)) + expect(onPaired, 'a link paired the app without being asked').not.toHaveBeenCalled() + + // And the user is told what they would be trusting, by name. + const heading = await screen.findByRole('heading') + expect(heading.textContent).toMatch(/connect this box\?/i) + expect(await screen.findByRole('button', { name: /connect this box/i })).toBeTruthy() + expect(await screen.findByRole('button', { name: /not now/i })).toBeTruthy() + }) + + it('names the box it points at, so two boxes look different', async () => { + render(Pair, { props: { fragment: fragmentFor(ATTACKER_KEY), onPaired: vi.fn() } }) + + // Six hex characters of the key's digest. Nobody memorises it, but it is + // what makes "this is not the box on my wall" noticeable at all. + const body = document.body.textContent ?? '' + await vi.waitFor(() => expect(body.length).toBeGreaterThan(0)) + await new Promise((r) => setTimeout(r, 50)) + expect(document.body.textContent).toMatch(/[0-9A-F]{6}/) + }) + + it('refuses a link that is not an FTW code, without pairing anything', async () => { + const onPaired = vi.fn() + render(Pair, { props: { fragment: '#not-a-pairing-code', onPaired } }) + + await new Promise((r) => setTimeout(r, 50)) + expect(onPaired).not.toHaveBeenCalled() + expect(document.body.textContent).toMatch(/not an FTW pairing code/i) + }) +})