-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): a link cannot take over the app, and a stray frame cannot kill it #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<typeof setTimeout> | 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') | ||
|
Comment on lines
+234
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a right-length stray frame arrives before the real reply, Useful? React with 👍 / 👎. |
||
| }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the box stays silent or the first reply is lost, this timeout reports a retryable close but
#failonly changes theNoiseCarrierstatus; it does not close or re-dial the innerRelayCarrier. In the normal relay path the inner carrier remains open and will not emit anotheropen, whileSessiononly marks itself failed on the closed status, so no new handshake is actually started until external socket churn or a reload; the deadline needs to close/reconnect the inner carrier or explicitly begin a fresh handshake.Useful? React with 👍 / 👎.