Public: anyone can read this; the crew can change it.
Attack the signed path that has already merged: src/attack.test.ts #56
Check · Review · Land · Live done — Landed and live in production.
torque/relay:torque/28-attack-the-signed-path-that-has-already → main 1 commit fork 2 threads · 2 unresolved
2 files
changed +467
−8
src/attack.test.ts added +447
| @@ -0,0 +1,447 @@ | ||
| 1 | /** | |
| 2 | * The attacks on the signed path, in one file. Each test is named for what | |
| 3 | * it tries and asserts the exact verdict string, so a refusal that quietly | |
| 4 | * becomes a different refusal — `bad_signature` where `malformed` stood — | |
| 5 | * fails here rather than passing as "still refused". | |
| 6 | * | |
| 7 | * Read it as a list of claims about what relay refuses. Nothing in it is | |
| 8 | * the happy path in an angrier voice: every case is one an attacker with a | |
| 9 | * captured request and no secret would actually send. | |
| 10 | * | |
| 11 | * What it covers: `src/sign.ts`, `src/nonce.ts` and the identity | |
| 12 | * `src/apps.ts` derives. What it cannot reach is listed at the end of the | |
| 13 | * file, with the issue that owns each gap. | |
| 14 | */ | |
| 15 | ||
| 16 | import { expect, test } from "bun:test"; | |
| 17 | import { createHmac } from "node:crypto"; | |
| 18 | import { mkdtempSync } from "node:fs"; | |
| 19 | import { tmpdir } from "node:os"; | |
| 20 | import { join } from "node:path"; | |
| 21 | import { registerApp, rotateAppKey, SOURCE_RE } from "./actions.ts"; | |
| 22 | import { appSecret } from "./apps.ts"; | |
| 23 | import { openDb } from "./db.ts"; | |
| 24 | import { remember } from "./nonce.ts"; | |
| 25 | import { | |
| 26 | APP_RE, | |
| 27 | canonical, | |
| 28 | NONCE_RE, | |
| 29 | signRequest, | |
| 30 | verifyRequest, | |
| 31 | WINDOW_MS, | |
| 32 | type Parts, | |
| 33 | } from "./sign.ts"; | |
| 34 | import type { Who } from "./who.ts"; | |
| 35 | ||
| 36 | const SECRET = "a-secret-only-alpha-holds"; | |
| 37 | const NOW = 1_700_000_000_000; | |
| 38 | ||
| 39 | /** The request an attacker captured: valid, signed, already delivered. */ | |
| 40 | const real: Parts = { | |
| 41 | method: "POST", | |
| 42 | path: "/api/events", | |
| 43 | app: "alpha", | |
| 44 | timestamp: String(NOW), | |
| 45 | nonce: "n0nce-1234-ABCD", | |
| 46 | body: '{"kind":"order.placed","subject":"order-1"}', | |
| 47 | }; | |
| 48 | const stolen = signRequest(real, SECRET); | |
| 49 | ||
| 50 | /** The captured request with one thing changed, verified against its signature. */ | |
| 51 | const replay = (change: Partial<Parts>, signature = stolen): string => | |
| 52 | verifyRequest({ ...real, ...change }, SECRET, signature, NOW); | |
| 53 | ||
| 54 | test("the captured request is genuine, so every refusal below is the change, not the capture", () => { | |
| 55 | expect(verifyRequest(real, SECRET, stolen, NOW)).toBe("ok"); | |
| 56 | }); | |
| 57 | ||
| 58 | // ---------------------------------------------------------------- identity | |
| 59 | ||
| 60 | test("identity confusion: a signature made by alpha, presented as beta", () => { | |
| 61 | // The realistic attempt: beta's secret is not alpha's, so the compare fails. | |
| 62 | expect( | |
| 63 | verifyRequest({ ...real, app: "beta" }, "a-secret-only-beta-holds", stolen, NOW), | |
| 64 | ).toBe("bad_signature"); | |
| 65 | // And the worst case, which is the claim that matters: even if the two | |
| 66 | // apps somehow shared a secret, the app id is inside the canonical | |
| 67 | // string, so swapping it still cannot be `ok`. | |
| 68 | expect(replay({ app: "beta" })).toBe("bad_signature"); | |
| 69 | }); | |
| 70 | ||
| 71 | test("identity confusion: the same app id at another generation's secret", () => { | |
| 72 | const key = "an-identity-key-for-this-test-only"; | |
| 73 | const gen1 = appSecret("alpha", 1, key); | |
| 74 | const gen2 = appSecret("alpha", 2, key); | |
| 75 | const signedAtGen1 = signRequest(real, gen1); | |
| 76 | expect(verifyRequest(real, gen1, signedAtGen1, NOW)).toBe("ok"); | |
| 77 | expect(verifyRequest(real, gen2, signedAtGen1, NOW)).toBe("bad_signature"); | |
| 78 | }); | |
| 79 | ||
| 80 | test("identity confusion: no two apps can derive one secret, because an id cannot carry the ':' the derivation joins on", () => { | |
| 81 | // appSecret hashes `relay-app:<id>:<generation>`. If an id could contain | |
| 82 | // a colon, ("alpha:1", 2) and ("alpha", "1:2") would be the same string. | |
| 83 | expect(APP_RE.test("alpha:1")).toBe(false); | |
| 84 | expect(appSecret("alpha", 12, "k")).not.toBe(appSecret("alpha", 1, "k")); | |
| 85 | }); | |
| 86 | ||
| 87 | // -------------------------------------------------------------------- body | |
| 88 | ||
| 89 | test("body confusion: bytes that differ from what was hashed are all bad_signature", () => { | |
| 90 | const original = real.body as string; | |
| 91 | const mutations: Record<string, string> = { | |
| 92 | "a trailing newline": `${original}\n`, | |
| 93 | "the same JSON, keys re-serialised in another order": | |
| 94 | '{"subject":"order-1","kind":"order.placed"}', | |
| 95 | "CRLF for LF": '{"kind":"order.placed",\r\n"subject":"order-1"}', | |
| 96 | "a zero-width space inside a value": | |
| 97 | '{"kind":"order.placed","subject":"order-1"}', | |
| 98 | "whitespace JSON parses away": ' {"kind":"order.placed","subject":"order-1"}', | |
| 99 | }; | |
| 100 | for (const [what, body] of Object.entries(mutations)) { | |
| 101 | expect(`${what}: ${replay({ body })}`).toBe(`${what}: bad_signature`); | |
| 102 | } | |
| 103 | }); | |
| 104 | ||
| 105 | test("body confusion: an empty body against a hash of {}", () => { | |
| 106 | const braces = { ...real, body: "{}" }; | |
| 107 | const signed = signRequest(braces, SECRET); | |
| 108 | expect(verifyRequest(braces, SECRET, signed, NOW)).toBe("ok"); | |
| 109 | expect(verifyRequest({ ...braces, body: "" }, SECRET, signed, NOW)).toBe( | |
| 110 | "bad_signature", | |
| 111 | ); | |
| 112 | // Both directions: a hash of "" does not cover "{}" either. | |
| 113 | const empty = { ...real, body: "" }; | |
| 114 | expect( | |
| 115 | verifyRequest({ ...empty, body: "{}" }, SECRET, signRequest(empty, SECRET), NOW), | |
| 116 | ).toBe("bad_signature"); | |
| 117 | }); | |
| 118 | ||
| 119 | test("body confusion: a string body is hashed as UTF-8, so the door must hash the bytes it read", () => { | |
| 120 | // Not a bug in this module, and not a way past it: a lone surrogate | |
| 121 | // cannot survive a UTF-8 decode, so `Buffer.from(s, "utf8")` replaces it | |
| 122 | // with U+FFFD and the string hashes as those three bytes. It means the | |
| 123 | // door (#8) must pass `readBody`'s raw bytes to `verifyRequest`, never a | |
| 124 | // string it decoded first — decoding is lossy and the client signed | |
| 125 | // bytes. Passing a Uint8Array here is exact. | |
| 126 | const asText = signRequest({ ...real, body: "\ud800" }, SECRET); | |
| 127 | const asBytes = signRequest( | |
| 128 | { ...real, body: new Uint8Array([0xef, 0xbf, 0xbd]) }, | |
| 129 | SECRET, | |
| 130 | ); | |
| 131 | expect(asText).toBe(asBytes); | |
| 132 | }); | |
| 133 | ||
| 134 | // -------------------------------------------------------------------- path | |
| 135 | ||
| 136 | test("path confusion: a near-miss path never verifies against the real one", () => { | |
| 137 | const paths: Record<string, string> = { | |
| 138 | "a trailing slash": "/api/events/", | |
| 139 | "a doubled slash": "/api//events", | |
| 140 | "a percent-encoded letter": "/api/%65vents", | |
| 141 | "a capitalised path": "/API/events", | |
| 142 | }; | |
| 143 | for (const [what, path] of Object.entries(paths)) { | |
| 144 | expect(`${what}: ${replay({ path })}`).toBe(`${what}: bad_signature`); | |
| 145 | } | |
| 146 | // A Cyrillic 'е' for the Latin one: rejected before it is ever compared, | |
| 147 | // because the path is ASCII printable only. | |
| 148 | expect(replay({ path: "/api/еvents" })).toBe("malformed"); | |
| 149 | }); | |
| 150 | ||
| 151 | test("path confusion: a query string is bytes in the path like any other, and the door must decide whether it is signed", () => { | |
| 152 | // The module's contract is that a signed POST carries no query. Nothing | |
| 153 | // here strips one: `/api/events?admin=1` is simply a different path and | |
| 154 | // fails against the real signature. The live risk is a door that | |
| 155 | // verifies `url.pathname` while routing on the full URL, which would | |
| 156 | // leave the query unsigned and attacker-controlled — that belongs to #8, | |
| 157 | // which must verify and route on the same string. | |
| 158 | expect(replay({ path: "/api/events?admin=1" })).toBe("bad_signature"); | |
| 159 | const withQuery = { ...real, path: "/api/events?admin=1" }; | |
| 160 | expect( | |
| 161 | verifyRequest(withQuery, SECRET, signRequest(withQuery, SECRET), NOW), | |
| 162 | ).toBe("ok"); | |
| 163 | }); | |
| 164 | ||
| 165 | // ------------------------------------------------------------------ fields | |
| 166 | ||
| 167 | test("field abuse: a timestamp that is not plain digits is malformed, and none of it throws", () => { | |
| 168 | const bad: Record<string, string> = { | |
| 169 | "a leading plus": "+1700000000000", | |
| 170 | "empty": "", | |
| 171 | "sixteen digits": "1".repeat(16), | |
| 172 | "a space": " 1700000000000", | |
| 173 | "exponent notation": "1.7e12", | |
| 174 | "a negative": "-1700000000000", | |
| 175 | }; | |
| 176 | for (const [what, timestamp] of Object.entries(bad)) { | |
| 177 | expect(() => replay({ timestamp })).not.toThrow(); | |
| 178 | expect(`${what}: ${replay({ timestamp })}`).toBe(`${what}: malformed`); | |
| 179 | } | |
| 180 | }); | |
| 181 | ||
| 182 | test("field abuse: leading zeros are digits, so they are not malformed — but they are different bytes", () => { | |
| 183 | // The honest verdict, against what the issue guessed: `^\d{1,15}$` | |
| 184 | // accepts "01700000000000", and refusing it would not close anything, | |
| 185 | // because the timestamp is signed as the exact string it is. The claim | |
| 186 | // worth pinning is that `Number()` is used only to measure the window | |
| 187 | // and never to compare: a signature over "1700000000000" does not carry | |
| 188 | // over to a leading-zero spelling of the same instant. | |
| 189 | const padded = { ...real, timestamp: `0${NOW}` }; | |
| 190 | expect(verifyRequest(padded, SECRET, stolen, NOW)).toBe("bad_signature"); | |
| 191 | // It is its own request, with its own signature, and its own nonce | |
| 192 | // claim — so it buys an attacker nothing without the secret. | |
| 193 | expect(verifyRequest(padded, SECRET, signRequest(padded, SECRET), NOW)).toBe("ok"); | |
| 194 | }); | |
| 195 | ||
| 196 | test("field abuse: the nonce length boundaries hold at 8 and 64, and refuse at 7 and 65", () => { | |
| 197 | const at = (n: number) => ({ ...real, nonce: "a".repeat(n) }); | |
| 198 | expect(replay({ nonce: "a".repeat(7) })).toBe("malformed"); | |
| 199 | expect(replay({ nonce: "a".repeat(65) })).toBe("malformed"); | |
| 200 | // The ends of the range are legal, and must stay legal: a client that | |
| 201 | // sends the shortest allowed nonce is not an attacker. | |
| 202 | expect(verifyRequest(at(8), SECRET, signRequest(at(8), SECRET), NOW)).toBe("ok"); | |
| 203 | expect(verifyRequest(at(64), SECRET, signRequest(at(64), SECRET), NOW)).toBe("ok"); | |
| 204 | }); | |
| 205 | ||
| 206 | test("field abuse: a base64 nonce is refused where a base64url one is required", () => { | |
| 207 | const db = openDb(mkdtempSync(join(tmpdir(), "attack-nonce-"))); | |
| 208 | for (const ch of ["+", "/", "="]) { | |
| 209 | const nonce = `n0nce12${ch}abcd`; | |
| 210 | expect(NONCE_RE.test(nonce)).toBe(false); | |
| 211 | expect(() => replay({ nonce })).not.toThrow(); | |
| 212 | expect(`${ch}: ${replay({ nonce })}`).toBe(`${ch}: malformed`); | |
| 213 | // And `src/nonce.ts` refuses to remember what `src/sign.ts` would not | |
| 214 | // verify, so a nonce the verifier rejects can never take a row. | |
| 215 | expect(remember(db, "alpha", nonce, NOW)).toBe("malformed"); | |
| 216 | } | |
| 217 | expect( | |
| 218 | (db.query("SELECT count(*) AS n FROM nonce").get() as { n: number }).n, | |
| 219 | ).toBe(0); | |
| 220 | }); | |
| 221 | ||
| 222 | test("field abuse: an app id with a capital is malformed", () => { | |
| 223 | expect(replay({ app: "Alpha" })).toBe("malformed"); | |
| 224 | expect(replay({ app: "alpha_beta" })).toBe("malformed"); | |
| 225 | expect(replay({ app: "a".repeat(33) })).toBe("malformed"); | |
| 226 | }); | |
| 227 | ||
| 228 | test("field abuse: a trailing hyphen is a legal app id, by the same rule that registers one", () => { | |
| 229 | // The issue expected `malformed`. It is not, and tightening only | |
| 230 | // `sign.ts` would be worse than leaving it: `registerApp` admits | |
| 231 | // `alpha-` under its own copy of the rule, so an app could be registered | |
| 232 | // that could never sign. A trailing hyphen confuses nothing — ids are | |
| 233 | // whole fields in a `\n`-joined string, and "alpha-" and "alpha" are two | |
| 234 | // different apps. The regression worth catching is the two rules | |
| 235 | // drifting apart, so this asserts they are the same rule. | |
| 236 | expect(APP_RE.source).toBe(SOURCE_RE.source); | |
| 237 | expect(APP_RE.test("alpha-")).toBe(true); | |
| 238 | const hyphen = { ...real, app: "alpha-" }; | |
| 239 | expect(verifyRequest(hyphen, SECRET, signRequest(hyphen, SECRET), NOW)).toBe("ok"); | |
| 240 | expect(replay({ app: "alpha-" })).toBe("bad_signature"); | |
| 241 | }); | |
| 242 | ||
| 243 | test("field abuse: a newline in any field cannot re-cut the canonical string", () => { | |
| 244 | // `relay1\nPOST\n/api/events\nalpha\n...` — a field that could carry a | |
| 245 | // newline could move the boundary between two others. | |
| 246 | for (const field of ["app", "nonce", "path"] as const) { | |
| 247 | const smuggled = { ...real, [field]: `${real[field]}\nalpha` }; | |
| 248 | expect(verifyRequest(smuggled, SECRET, stolen, NOW)).toBe("malformed"); | |
| 249 | expect(() => canonical(smuggled)).toThrow(); | |
| 250 | } | |
| 251 | expect(replay({ method: "POST\nGET" })).toBe("malformed"); | |
| 252 | }); | |
| 253 | ||
| 254 | // --------------------------------------------------------- signature shape | |
| 255 | ||
| 256 | test("signature shape: every wrong signature is bad_signature, never ok and never a throw", () => { | |
| 257 | const signatures: Record<string, string> = { | |
| 258 | "the right length, the wrong secret": signRequest(real, "not-the-secret"), | |
| 259 | "truncated to 42": stolen.slice(0, 42), | |
| 260 | "padded to 44": `${stolen}A`, | |
| 261 | "base64 padding": `${stolen.slice(0, 42)}=`, | |
| 262 | "empty": "", | |
| 263 | "43 characters of the wrong alphabet": "!".repeat(43), | |
| 264 | "the canonical string itself": "relay1", | |
| 265 | }; | |
| 266 | for (const [what, signature] of Object.entries(signatures)) { | |
| 267 | expect(() => verifyRequest(real, SECRET, signature, NOW)).not.toThrow(); | |
| 268 | expect(`${what}: ${verifyRequest(real, SECRET, signature, NOW)}`).toBe( | |
| 269 | `${what}: bad_signature`, | |
| 270 | ); | |
| 271 | } | |
| 272 | }); | |
| 273 | ||
| 274 | test("signature shape: timingSafeEqual is never handed two different lengths", () => { | |
| 275 | // It throws on mismatched lengths, and a throw at the door is a refusal | |
| 276 | // an attacker can time and distinguish. The shape check is what prevents | |
| 277 | // it: 43 base64url characters decode to exactly 32 bytes — the digest's | |
| 278 | // length — for every character in the alphabet, including the last one, | |
| 279 | // whose low bits are dropped. | |
| 280 | const alphabet = | |
| 281 | "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_"; | |
| 282 | for (const ch of alphabet) { | |
| 283 | const candidate = `${"A".repeat(42)}${ch}`; | |
| 284 | expect(Buffer.from(candidate, "base64url").length).toBe(32); | |
| 285 | expect(verifyRequest(real, SECRET, candidate, NOW)).toBe("bad_signature"); | |
| 286 | } | |
| 287 | }); | |
| 288 | ||
| 289 | // ------------------------------------------------------------------- clock | |
| 290 | ||
| 291 | test("clock: exactly at the window is fresh, one millisecond past it is stale, both directions", () => { | |
| 292 | const at = (offset: number) => { | |
| 293 | const parts = { ...real, timestamp: String(NOW + offset) }; | |
| 294 | return verifyRequest(parts, SECRET, signRequest(parts, SECRET), NOW); | |
| 295 | }; | |
| 296 | expect(at(-WINDOW_MS)).toBe("ok"); | |
| 297 | expect(at(WINDOW_MS)).toBe("ok"); | |
| 298 | expect(at(-WINDOW_MS - 1)).toBe("stale"); | |
| 299 | expect(at(WINDOW_MS + 1)).toBe("stale"); | |
| 300 | // A clock a year ahead is refused the same way, not accepted as "future". | |
| 301 | expect(at(365 * 24 * 60 * 60 * 1000)).toBe("stale"); | |
| 302 | expect(at(999_999_999_999_999 - NOW)).toBe("stale"); | |
| 303 | }); | |
| 304 | ||
| 305 | test("clock: stale is decided before the signature is compared, so an expired request is no oracle", () => { | |
| 306 | // If the compare ran first, an attacker could replay one expired capture | |
| 307 | // with guessed signatures and read `stale` versus `bad_signature` as a | |
| 308 | // yes/no on each guess. The verdict must not depend on the signature's | |
| 309 | // correctness at all. | |
| 310 | const expired = { ...real, timestamp: String(NOW - WINDOW_MS - 1) }; | |
| 311 | const correct = signRequest(expired, SECRET); | |
| 312 | const wrong = signRequest(expired, "not-the-secret"); | |
| 313 | expect(verifyRequest(expired, SECRET, correct, NOW)).toBe("stale"); | |
| 314 | expect(verifyRequest(expired, SECRET, wrong, NOW)).toBe("stale"); | |
| 315 | expect(verifyRequest(expired, SECRET, "A".repeat(43), NOW)).toBe("stale"); | |
| 316 | }); | |
| 317 | ||
| 318 | test("clock: the order of verdicts is malformed, then shape, then stale, then the compare", () => { | |
| 319 | const expired = String(NOW - WINDOW_MS - 1); | |
| 320 | // Malformed parts outrank an expired clock. | |
| 321 | expect( | |
| 322 | verifyRequest({ ...real, app: "Alpha", timestamp: expired }, SECRET, stolen, NOW), | |
| 323 | ).toBe("malformed"); | |
| 324 | // A misshapen signature outranks it too. That is a length check on the | |
| 325 | // attacker's own input, not a comparison against the real digest, so it | |
| 326 | // tells them nothing they did not already know. | |
| 327 | expect( | |
| 328 | verifyRequest({ ...real, timestamp: expired }, SECRET, "short", NOW), | |
| 329 | ).toBe("bad_signature"); | |
| 330 | }); | |
| 331 | ||
| 332 | // ------------------------------------------------------------------ replay | |
| 333 | ||
| 334 | test("replay: a nonce remembered under one generation stays remembered after the app rotates", () => { | |
| 335 | const db = openDb(mkdtempSync(join(tmpdir(), "attack-rotate-"))); | |
| 336 | const key = "an-identity-key-for-this-test-only"; | |
| 337 | const root: Who = { role: "admin", id: "u0", name: "root" }; | |
| 338 | const registered = registerApp(db, root, { id: "alpha", name: "Alpha" }, key); | |
| 339 | expect(registered.ok).toBe(true); | |
| 340 | if (!registered.ok) return; | |
| 341 | ||
| 342 | const nonce = "rotation-nonce-01"; | |
| 343 | expect(remember(db, "alpha", nonce, NOW)).toBe("fresh"); | |
| 344 | ||
| 345 | const rotated = rotateAppKey(db, root, { id: "alpha" }, key); | |
| 346 | expect(rotated.ok).toBe(true); | |
| 347 | if (!rotated.ok) return; | |
| 348 | expect(rotated.value.key).not.toBe(registered.value.key); | |
| 349 | ||
| 350 | // The secret changed; the memory did not, because `remember` keys on the | |
| 351 | // app id. Rotating a key must not hand an attacker a fresh replay window | |
| 352 | // on every nonce the old key ever used. | |
| 353 | expect(remember(db, "alpha", nonce, NOW)).toBe("replay"); | |
| 354 | }); | |
| 355 | ||
| 356 | test("replay: the nonce a rotation cannot reuse is the exact string, byte for byte", () => { | |
| 357 | const db = openDb(mkdtempSync(join(tmpdir(), "attack-case-"))); | |
| 358 | expect(remember(db, "alpha", "CaseSensitive-01", NOW)).toBe("fresh"); | |
| 359 | // A different case is a different nonce — which is safe, not a hole: the | |
| 360 | // nonce is signed, so spelling it differently needs the secret anyway. | |
| 361 | expect(remember(db, "alpha", "casesensitive-01", NOW)).toBe("fresh"); | |
| 362 | expect(remember(db, "alpha", "CaseSensitive-01", NOW)).toBe("replay"); | |
| 363 | }); | |
| 364 | ||
| 365 | // --------------------------------------------------------- unsafe defaults | |
| 366 | ||
| 367 | test("unsafe default, found: the empty secret verified anything, and now refuses everything", () => { | |
| 368 | // This attack landed. HMAC accepts any key, the empty one included, so | |
| 369 | // `verifyRequest(parts, "", attackerSignature)` returned `ok` — a door | |
| 370 | // holding an unset environment variable would have authenticated the | |
| 371 | // whole internet as any app. `src/sign.ts` now refuses an empty secret | |
| 372 | // outright, and signing with one throws rather than producing a | |
| 373 | // signature nobody should trust. | |
| 374 | // This is the signature an attacker sends: HMAC-SHA256 of the canonical | |
| 375 | // string under the empty key, which anyone can compute. Before the fix | |
| 376 | // this exact string verified `ok`. | |
| 377 | const underEmptyKey = createHmac("sha256", "") | |
| 378 | .update(canonical(real)) | |
| 379 | .digest("base64url"); | |
| 380 | expect(underEmptyKey).toBe("XrIb7G6jJH2KvU1Kngg6NKCpSt9DRhTaGLk4RZ4kuyM"); | |
| 381 | expect(verifyRequest(real, "", underEmptyKey, NOW)).toBe("bad_signature"); | |
| 382 | // Nor can the secret go missing on the signing side and produce | |
| 383 | // something a reader would mistake for a real signature. | |
| 384 | expect(() => signRequest(real, "")).toThrow(); | |
| 385 | // A real secret still verifies: the refusal is the empty key, not a | |
| 386 | // blanket one. | |
| 387 | expect(verifyRequest(real, SECRET, stolen, NOW)).toBe("ok"); | |
| 388 | }); | |
| 389 | ||
| 390 | test("unsafe default: an app secret cannot be derived without an identity key", () => { | |
| 391 | // The other half of the same hole: if `appSecret` fell back to "" when | |
| 392 | // the forge configured no identity key, every reader of this repository | |
| 393 | // could compute every app's secret. | |
| 394 | expect(() => appSecret("alpha", 1, "")).toThrow(); | |
| 395 | }); | |
| 396 | ||
| 397 | test("a one-byte key and a one-kilobyte key both sign, and that is HMAC, not a bug", () => { | |
| 398 | // HMAC-SHA256 zero-pads a key shorter than its 64-byte block and hashes | |
| 399 | // one longer than it, so any non-empty key is accepted and the digest is | |
| 400 | // always 32 bytes. Do not file this as a weakness in `sign.ts`: key | |
| 401 | // strength is `src/apps.ts`'s job, and it derives 43 base64url | |
| 402 | // characters — 256 bits — every time. | |
| 403 | const tiny = { secret: "x", parts: real }; | |
| 404 | const huge = { secret: "k".repeat(1024), parts: real }; | |
| 405 | expect(signRequest(tiny.parts, tiny.secret)).toHaveLength(43); | |
| 406 | expect(signRequest(huge.parts, huge.secret)).toHaveLength(43); | |
| 407 | expect(signRequest(real, tiny.secret)).not.toBe(signRequest(real, huge.secret)); | |
| 408 | expect(appSecret("alpha", 1, "an-identity-key-for-this-test-only")).toHaveLength(43); | |
| 409 | }); | |
| 410 | ||
| 411 | /* | |
| 412 | * What cannot be attacked from here, and who owns it. | |
| 413 | * | |
| 414 | * This file reaches `verifyRequest`, `remember` and `appSecret` directly. | |
| 415 | * Everything below needs the door, and is not covered by anything in this | |
| 416 | * repository until the issue named takes it: | |
| 417 | * | |
| 418 | * - The door's status codes. Which verdict becomes 400, 401 or 403, that | |
| 419 | * the body never repeats the signature back, and that `malformed` and | |
| 420 | * `bad_signature` are not distinguishable to an attacker who should | |
| 421 | * only learn "no" — #8. | |
| 422 | * - Where the door gets each part. The path it verifies versus the path | |
| 423 | * it routes on, whether it strips a query, whether it hashes the raw | |
| 424 | * bytes of `readBody` or a decoded string, and the body-size cap | |
| 425 | * applied before any hashing — #8. | |
| 426 | * - `remember` called on the request path at all: verification and the | |
| 427 | * replay claim are separate functions here, and only the door orders | |
| 428 | * them (verify first, then claim the nonce, then act) — #8. | |
| 429 | * - Revocation and generation lookup: that a revoked app's valid | |
| 430 | * signature is refused, and that the door verifies against the current | |
| 431 | * generation's secret only — #8. | |
| 432 | * - The per-app ration: how many refusals an unknown or wrong-signature | |
| 433 | * caller may spend before the bucket answers 429, and that a refusal | |
| 434 | * costs a token so signature guessing is not free — #24. | |
| 435 | * - Replay over the network: concurrent delivery of one captured request | |
| 436 | * to two processes on the same database file, which `remember`'s | |
| 437 | * single INSERT is written for but only a real race can demonstrate — | |
| 438 | * #8. | |
| 439 | * - TLS, and everything that assumes it: the forge terminates it, so a | |
| 440 | * capture on the wire is out of this repository's hands entirely. The | |
| 441 | * signature is what survives a proxy; headers and Host are not signed | |
| 442 | * for exactly that reason. | |
| 443 | * - Timing of the compare itself. `timingSafeEqual` is constant-time by | |
| 444 | * construction and this file asserts it is never given mismatched | |
| 445 | * lengths, but a statistical timing measurement is not something a | |
| 446 | * unit test can make a claim about. | |
| 447 | */ |
src/sign.ts +20 −8
| @@ -72,16 +72,27 @@ export function canonical(parts: Parts): string { | ||
| 72 | 72 | ].join("\n"); |
| 73 | 73 | } |
| 74 | 74 | |
| 75 | /** base64url(HMAC-SHA256(secret, canonical(parts))). Throws on a malformed part. */ | |
| 76 | export const signRequest = (parts: Parts, secret: string): string => | |
| 77 | createHmac("sha256", secret).update(canonical(parts)).digest("base64url"); | |
| 75 | /** | |
| 76 | * base64url(HMAC-SHA256(secret, canonical(parts))). Throws on a malformed | |
| 77 | * part, and on the empty secret: HMAC takes any key, so a caller whose | |
| 78 | * secret went missing — an unset environment variable, an app row read | |
| 79 | * before it was derived — would otherwise sign happily with nothing. | |
| 80 | */ | |
| 81 | export function signRequest(parts: Parts, secret: string): string { | |
| 82 | if (secret === "") throw new TypeError("sign: empty secret"); | |
| 83 | return createHmac("sha256", secret) | |
| 84 | .update(canonical(parts)) | |
| 85 | .digest("base64url"); | |
| 86 | } | |
| 78 | 87 | |
| 79 | 88 | /** |
| 80 | * Never throws. `malformed` is decided first, from the parts alone; a | |
| 81 | * signature that isn't 43 base64url characters is `bad_signature` rather | |
| 82 | * than something that fails to decode; a timestamp outside the 300s window | |
| 83 | * is `stale`, decided before the constant-time compare that would | |
| 84 | * otherwise report `ok` or `bad_signature`. | |
| 89 | * Never throws. `malformed` is decided first, from the parts alone; an | |
| 90 | * empty secret refuses everything, because HMAC under the empty key is a | |
| 91 | * key the attacker holds too; a signature that isn't 43 base64url | |
| 92 | * characters is `bad_signature` rather than something that fails to | |
| 93 | * decode; a timestamp outside the 300s window is `stale`, decided before | |
| 94 | * the constant-time compare that would otherwise report `ok` or | |
| 95 | * `bad_signature`. | |
| 85 | 96 | */ |
| 86 | 97 | export function verifyRequest( |
| 87 | 98 | parts: Parts, |
| @@ -90,6 +101,7 @@ export function verifyRequest( | ||
| 90 | 101 | now = Date.now(), |
| 91 | 102 | ): VerifyResult { |
| 92 | 103 | if (malformed(parts)) return "malformed"; |
| 104 | if (secret === "") return "bad_signature"; | |
_(A stray one-character comment from me sits on line 102 of this file: it was a probe while I worked out that this forge anchors reviews to a line, and the API has no way to delete it. Ignore it; this is the review.)_ Approved. I ran it rather than trusting the check: Against the bar: no new surface, no dependency, no secret (the Four notes for #8, none blocking:
The thing I want #8 to carry forward from this file is the sentence in the UTF-8 body test: pass |
||
| 93 | 105 | if (!SIGNATURE_RE.test(signature)) return "bad_signature"; |
| 94 | 106 | if (Math.abs(now - Number(parts.timestamp)) > WINDOW_MS) return "stale"; |
| 95 | 107 | const expected = Buffer.from(signRequest(parts, secret), "base64url"); |
x