From 1d0b4beb4cfddf307c8f2569b16bc7c194580d14 Mon Sep 17 00:00:00 2001 From: majcosta Date: Tue, 28 Jul 2026 10:06:19 -0300 Subject: [PATCH] Sanitize every field the channel summary quotes, not just the handle The endpoint is public and unauthenticated, so the whole uploaded file is attacker-chosen, not only the part the client copied from Ja2.ini. The build field sat inside backticks a backtick closes, and the access-violation text went in raw, so either could carry markdown or a link into the channel. One clean(): printable ASCII minus what Discord reads as markup or a URL, length-capped. It replaces the handle's own stripping and absorbs the .trim() the field getters did, which also drops the CR that "(.+)$" captures off a CRLF report. Co-Authored-By: Claude Opus 5 --- crash-telemetry/README.md | 8 ++++++-- crash-telemetry/test.mjs | 7 ++++++- crash-telemetry/worker.js | 18 ++++++++++++------ 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/crash-telemetry/README.md b/crash-telemetry/README.md index 45ecc1113..e08ec1946 100644 --- a/crash-telemetry/README.md +++ b/crash-telemetry/README.md @@ -96,6 +96,10 @@ a millisecond of CPU — the time spent waiting on Discord is not metered. No authentication. The endpoint is public and its URL ships in every player's `Ja2.ini`, so assume it will eventually be found; the size and `*** CRASH` checks -only keep out drive-by scanners. Report contents are attacker-controlled text, which -is why the summary line strips markdown from the handle and sends +only keep out drive-by scanners. Every field the summary line quotes is therefore +attacker-chosen, not just the handle: all of them go through `clean()`, which keeps +printable ASCII minus Discord's markup and link characters, and the post sends `allowed_mentions: {parse: []}`. Blast radius of abuse is a message we delete. + +The per-IP limiter does nothing against a distributed flood — that would cost +channel noise and the 100k/day request budget, not money. diff --git a/crash-telemetry/test.mjs b/crash-telemetry/test.mjs index cf06f9f14..b55f8e460 100644 --- a/crash-telemetry/test.mjs +++ b/crash-telemetry/test.mjs @@ -7,7 +7,7 @@ const REPORT = ` time 2026-07-26 10:41:02 UTC build 6a941c06 handle @marco*evil - access violation: read from 00000002 + access violation: read from 00000002 [x](https://evil.test) \` [0] 0071D5A0 [1] 006BE7DE `; @@ -33,6 +33,11 @@ assert.match(content, /C0000005/); assert.match(content, /read from 00000002/); assert.match(content, /build `6a941c06`/); assert.match(content, /marcoevil/); // markdown and @ stripped from the handle +// every field is attacker-chosen: no field may carry markup or a link into the +// channel, and none may close the backticks or bold the summary wraps it in. +assert.ok(!content.includes("]("), content); // no link syntax out of the report +assert.ok(!content.includes("://"), content); // and no bare URL either +assert.equal(content.match(/`/g).length, 4); // only the two pairs summarize() opens assert.deepEqual(JSON.parse(sent.body.get("payload_json")).allowed_mentions, { parse: [] }); assert.equal(await sent.body.get("files[0]").text(), REPORT); diff --git a/crash-telemetry/worker.js b/crash-telemetry/worker.js index e4614c6fd..0c3ca5b30 100644 --- a/crash-telemetry/worker.js +++ b/crash-telemetry/worker.js @@ -15,17 +15,23 @@ const MAX_BYTES = 64 * 1024; // reports run 2-8 KB; anything near this is not ours +// Every field below is lifted out of the uploaded file, and the endpoint is public +// and unauthenticated: treat all of it as attacker-chosen, not just the handle. +// Keep printable ASCII minus everything Discord reads as markup or a link, and cap +// the length, so nothing in a report can shape the message around it. +const clean = (s) => (s || "").replace(/[^A-Za-z0-9 .,+-]/g, "").slice(0, 64).trim(); + // Pull the header fields the client writes, for a one-line channel summary. // See writeExceptionBacktrace(): "*** CRASH code=... ***", then " ". function summarize(text) { - const field = (name) => (text.match(new RegExp(`^\\s+${name} (.+)$`, "m")) || [])[1]?.trim(); - const code = (text.match(/code=(\w+)/) || [])[1] || "????????"; - const av = (text.match(/access violation: (.+)/) || [])[1]?.trim(); + const field = (name) => (text.match(new RegExp(`^\\s+${name} (.+)$`, "m")) || [])[1]; + const code = clean((text.match(/code=(\w+)/) || [])[1]) || "????????"; + const av = clean((text.match(/access violation: (.+)/) || [])[1]); const parts = [`\`${code}\`${av ? ` (${av})` : ""}`]; - const build = field("build"); - const handle = field("handle"); + const build = clean(field("build")); + const handle = clean(field("handle")); if (build) parts.push(`build \`${build}\``); - if (handle) parts.push(`from **${handle.replace(/[`*_~|@]/g, "")}**`); // no pings, no markdown + if (handle) parts.push(`from **${handle}**`); return parts.join(" · "); }