From 85b49885256464b735923eaa771044706383718f Mon Sep 17 00:00:00 2001 From: Igor Soarez Date: Fri, 6 Nov 2026 09:20:16 +0000 Subject: broker: refuse uploads from outside the staging directory page.upload and a raw DOM.setFileInputFiles through cdp.call now have to name files whose real path is under /tmp/pi-chrome-uploads (PI_CHROME_UPLOAD_DIR), on the broker's host. Anyone holding the token could otherwise attach any file the user can read to a page. --- broker/broker.mjs | 8 +++++++ broker/upload-guard.mjs | 44 +++++++++++++++++++++++++++++++++++ package.json | 2 +- test-suite/unit/upload-guard.test.mjs | 41 ++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 broker/upload-guard.mjs create mode 100644 test-suite/unit/upload-guard.test.mjs diff --git a/broker/broker.mjs b/broker/broker.mjs index 4af8e9d..36fa302 100644 --- a/broker/broker.mjs +++ b/broker/broker.mjs @@ -27,6 +27,7 @@ import { execSync } from "node:child_process"; import { createHash, timingSafeEqual } from "node:crypto"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { assertUploadsStaged } from "./upload-guard.mjs"; const PKG_PATH = resolve(dirname(fileURLToPath(import.meta.url)), "..", "package.json"); @@ -236,6 +237,13 @@ async function handleCommand(request, response) { sendJson(response, 400, { ok: false, error: "Missing command action" }); return; } + try { + assertUploadsStaged(body.action, body.params, process.env.PI_CHROME_UPLOAD_DIR?.trim() || undefined); + } catch (error) { + log("warn", "command", { action: body.action, peer: request.socket.remoteAddress, ok: false, refused: "upload outside the staging directory" }); + sendJson(response, 403, { ok: false, error: error.message }); + return; + } const timeoutMs = Math.min(Math.max(Number(body.timeoutMs) || DEFAULT_TIMEOUT_MS, 1_000), MAX_TIMEOUT_MS); const started = Date.now(); const peer = request.socket.remoteAddress; diff --git a/broker/upload-guard.mjs b/broker/upload-guard.mjs new file mode 100644 index 0000000..dde8cdf --- /dev/null +++ b/broker/upload-guard.mjs @@ -0,0 +1,44 @@ +// Which local files a command may hand to the browser's file inputs. +// +// The broker runs on the browser's host, so a path in an upload command names a file on that +// machine, and nothing else checks it: the companion extension passes it to +// DOM.setFileInputFiles as given. Anyone holding the broker's token could attach any file the +// user can read to a page. Clients are told to stage uploads in one directory; this enforces it. +// +// Covers chrome_upload_file (page.upload) and the same CDP method through chrome_cdp (cdp.call). +// Paths are resolved with realpath first, so a symlink or ".." cannot lead out of the directory. + +import { realpathSync } from "node:fs"; +import { sep } from "node:path"; + +export const DEFAULT_UPLOAD_DIR = "/tmp/pi-chrome-uploads"; + +function resolveReal(path) { + try { + return realpathSync(path); + } catch { + return undefined; // missing: nothing to upload, and not ours to guess at + } +} + +function pathsOf(action, params) { + if (action === "page.upload") return Array.isArray(params?.paths) ? params.paths : []; + if (action === "cdp.call" && params?.method === "DOM.setFileInputFiles") { + const files = params.params?.files; + return Array.isArray(files) ? files : []; + } + return []; +} + +// Throws if a command would upload a file from outside uploadDir. +export function assertUploadsStaged(action, params, uploadDir = DEFAULT_UPLOAD_DIR) { + const paths = pathsOf(action, params); + if (paths.length === 0) return; + const root = resolveReal(uploadDir); + for (const path of paths) { + const real = typeof path === "string" && path.startsWith("/") ? resolveReal(path) : undefined; + if (root === undefined || real === undefined || !real.startsWith(root + sep)) { + throw new Error(`Uploads are limited to files in ${uploadDir} on the browser's host; "${path}" is not one. Copy the file there first.`); + } + } +} diff --git a/package.json b/package.json index 1e7a9f5..97ac4ea 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "pi-chrome", "version": "0.15.57", "scripts": { - "test": "node test-suite/unit/csp-eval.test.mjs && node test-suite/unit/automation-target.test.mjs && node test-suite/unit/session-cleanup.test.mjs && node test-suite/unit/background-policy.test.mjs && node test-suite/unit/input-reliability.test.mjs && node test-suite/unit/chrome-command.test.mjs && node test-suite/unit/bridge-resilience.test.mjs && node test-suite/unit/cdp-passthrough.test.mjs && node test-suite/unit/type-evidence.test.mjs && node test-suite/unit/hidden-input.test.mjs && node test-suite/unit/console-capture.test.mjs && node test-suite/unit/prompt-section.test.mjs", + "test": "node test-suite/unit/csp-eval.test.mjs && node test-suite/unit/automation-target.test.mjs && node test-suite/unit/session-cleanup.test.mjs && node test-suite/unit/background-policy.test.mjs && node test-suite/unit/input-reliability.test.mjs && node test-suite/unit/chrome-command.test.mjs && node test-suite/unit/bridge-resilience.test.mjs && node test-suite/unit/cdp-passthrough.test.mjs && node test-suite/unit/type-evidence.test.mjs && node test-suite/unit/hidden-input.test.mjs && node test-suite/unit/console-capture.test.mjs && node test-suite/unit/prompt-section.test.mjs && node test-suite/unit/upload-guard.test.mjs", "version": "node scripts/sync-manifest-version.js", "prepublishOnly": "node scripts/sync-manifest-version.js" }, diff --git a/test-suite/unit/upload-guard.test.mjs b/test-suite/unit/upload-guard.test.mjs new file mode 100644 index 0000000..b2a0daa --- /dev/null +++ b/test-suite/unit/upload-guard.test.mjs @@ -0,0 +1,41 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { test } from "node:test"; +import { assertUploadsStaged } from "../../broker/upload-guard.mjs"; + +const base = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "guard-"))); +const stage = path.join(base, "stage"); +fs.mkdirSync(stage); +fs.writeFileSync(path.join(stage, "ok.txt"), "x"); +fs.writeFileSync(path.join(base, "secret.txt"), "s"); +fs.symlinkSync(path.join(base, "secret.txt"), path.join(stage, "link.txt")); + +const upload = (...paths) => () => assertUploadsStaged("page.upload", { paths }, stage); + +test("a file in the staging directory is allowed", () => { + assert.doesNotThrow(upload(path.join(stage, "ok.txt"))); +}); +test("a file elsewhere, a ../ escape, a symlink out, a relative path and a missing file are refused", () => { + assert.throws(upload(path.join(base, "secret.txt")), /limited to files/); + assert.throws(upload(path.join(stage, "..", "secret.txt")), /limited to files/); + assert.throws(upload(path.join(stage, "link.txt")), /limited to files/); + assert.throws(upload("ok.txt"), /limited to files/); + assert.throws(upload(path.join(stage, "nope.txt")), /limited to files/); +}); +test("one bad path among good ones refuses the lot", () => { + assert.throws(upload(path.join(stage, "ok.txt"), "/etc/hosts"), /limited to files/); +}); +test("a missing staging directory refuses everything", () => { + assert.throws(() => assertUploadsStaged("page.upload", { paths: [path.join(stage, "ok.txt")] }, path.join(base, "none")), /limited to files/); +}); +test("raw DOM.setFileInputFiles through cdp.call is held to the same rule", () => { + const call = (files) => () => assertUploadsStaged("cdp.call", { method: "DOM.setFileInputFiles", params: { files } }, stage); + assert.doesNotThrow(call([path.join(stage, "ok.txt")])); + assert.throws(call(["/etc/hosts"]), /limited to files/); +}); +test("other commands are untouched", () => { + assert.doesNotThrow(() => assertUploadsStaged("page.click", { paths: ["/etc/hosts"] }, stage)); + assert.doesNotThrow(() => assertUploadsStaged("cdp.call", { method: "Runtime.evaluate", params: {} }, stage)); +}); -- cgit v1.2.3