From e58d2d58baa668a9a792617934807dd928518084 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 31 Oct 2026 11:02:45 +0100 Subject: [PATCH] Check WS Origin against the server's own origins, not the Host header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under a non-loopback --host the DNS-rebinding hostOk allowlist is skipped, and the old check compared Origin to the request's own Host header — a self-consistent attacker pair passed. Origins are now loopback names plus every local interface address. The alias.* claim from the same review was refuted by test (git does not expand aliases shadowing builtins) and kept as a regression net; DESIGN.md documents the round. --- DESIGN.md | 12 +++++ src/server/http.ts | 21 ++++++++- test/git.alias.security.test.ts | 60 +++++++++++++++++++++++++ test/http.wsorigin.security.test.ts | 68 +++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 test/git.alias.security.test.ts create mode 100644 test/http.wsorigin.security.test.ts diff --git a/DESIGN.md b/DESIGN.md index 6a29cd7..e51b64c 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -874,3 +874,15 @@ The Linux app was not affected by the image bug: its Chromium is confined with `--host-resolver-rules` and an unreachable `--proxy-server`, neither of which applies to `blob:` URLs, which never reach the network stack. There, TIFF and HEIC (which Chromium cannot decode) now open as the hex dump under a notice. + +## Security review, October 2026 + +An adversarial review (three parallel reviewers plus RED-test validation of the top +findings) re-walked the server surface. One finding was validated and fixed; several +were refuted or accepted as trade-offs. + +| Finding | Fix | +| --- | --- | +| The WebSocket Origin check compared `Origin` against the request's own `Host` header, so under a non-loopback `--host` (where the DNS-rebinding `hostOk` allowlist is skipped) a page could connect with a self-consistent attacker Host+Origin pair and get `101 Switching Protocols`. RED: `test/http.wsorigin.security.test.ts`. | Origin is now checked against the server's own origins — loopback names plus every local interface address (`os.networkInterfaces()`), never anything client-controlled. Deliberate consequence: the origin host must be a literal loopback or interface address; reaching the server via a DNS/mDNS name is rejected, because resolving a client-supplied hostname back to a local address would reopen the rebinding hole. | +| (Review) `alias.*`/`pager.*` in a hostile repo config were claimed to run commands. | Refuted by test: git does not expand an alias that shadows a builtin subcommand (`test/git.alias.security.test.ts` pins this, marker file stays empty). Kept as a regression net. | +| (Review) `HARDEN` overrides travel as `-c key=value` despite the GIT_CONFIG_KEY_n rule; token in the WS query string; missing-Origin bypass; regex CPU burn in the search worker; pty replay resync edge cases. | Not exploitable as written (values self-authored; query token is the documented WS mechanism; local DoS only). No change; recorded here for the next pass. | diff --git a/src/server/http.ts b/src/server/http.ts index 0f198e5..7f482c6 100644 --- a/src/server/http.ts +++ b/src/server/http.ts @@ -1,6 +1,7 @@ import type { Server, ServerWebSocket } from "bun"; import { randomBytes, timingSafeEqual } from "node:crypto"; import path from "node:path"; +import os from "node:os"; import { realpath, stat } from "node:fs/promises"; import { listDir, readForView, resolveSafe, imageMime, toRel, looksBinary, openRegular, MAX_TEXT_BYTES } from "./files.ts"; import { FileIndex } from "./index.ts"; @@ -66,6 +67,23 @@ export function startServer(opts: ServeOptions): { server: Server; u let port = 0; const loopback = ["127.0.0.1", "localhost", "::1"].includes(opts.host); + /** + * Every origin a legitimate browser page can have when talking to this server: + * loopback names plus every local interface address (a non-loopback --host can be + * reached under any of them). Never derived from the request's Host header, which + * the client controls — matching Origin against it made the cross-site WebSocket + * guard a no-op under DNS rebinding. + */ + const selfOrigins = (): Set => { + const set = new Set([`http://localhost:${port}`, `http://127.0.0.1:${port}`, `http://[::1]:${port}`]); + if (!loopback) { + for (const addrs of Object.values(os.networkInterfaces())) { + for (const a of addrs ?? []) set.add(`http://${a.address.includes(":") ? `[${a.address}]` : a.address}:${port}`); + } + } + return set; + }; + const broadcast = (msg: unknown) => { const s = JSON.stringify(msg); for (const ws of events) ws.send(s); @@ -262,8 +280,9 @@ export function startServer(opts: ServeOptions): { server: Server; u if (p.startsWith("/ws/")) { // Cross-site WebSocket hijacking guard: browsers always send Origin; it must be us. + // Compared against this server's own origins, never the client-controlled Host. const o = req.headers.get("origin"); - if (o && o !== `http://${req.headers.get("host")}`) return fail(403, "bad origin"); + if (o && !selfOrigins().has(o)) return fail(403, "bad origin"); if (p === "/ws/events") { return srv.upgrade(req, { data: { kind: "events" } }) ? undefined : fail(400, "upgrade failed"); } diff --git a/test/git.alias.security.test.ts b/test/git.alias.security.test.ts new file mode 100644 index 0000000..03753d0 --- /dev/null +++ b/test/git.alias.security.test.ts @@ -0,0 +1,60 @@ +import { afterAll, beforeAll, expect, test } from "bun:test"; +import { mkdtemp, rm, writeFile, mkdir, readFile, chmod, realpath } from "node:fs/promises"; +import { existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { Git } from "../src/server/git.ts"; + +// RED tests for the 2026-10 security review: a repository whose .git/config defines +// alias.* entries must not be able to make codeviewer's git calls run programs. +// git.ts's neutralize() only blanks filter./diff./merge. drivers (git.ts:191); +// alias.* is a separate config family that expands a shell command when the invoked +// subcommand name matches (e.g. `git status`, `git log` as called at git.ts:242-244). +let dir = ""; +let repo = ""; +let marker = ""; + +const sh = (cmd: string[], cwd = repo) => { + const r = Bun.spawnSync(["git", ...cmd], { cwd, stderr: "pipe" }); + if (r.exitCode !== 0) throw new Error(`git ${cmd.join(" ")}: ${r.stderr}`); + return r.stdout.toString(); +}; + +beforeAll(async () => { + dir = await realpath(await mkdtemp(path.join(tmpdir(), "cv-gitalias-"))); + repo = path.join(dir, "repo"); + marker = path.join(dir, "pwned.log"); + await mkdir(repo); + const evil = path.join(dir, "evil.sh"); + await writeFile(evil, `#!/bin/sh\necho "ran $*" >> '${marker}'\n`); + await chmod(evil, 0o755); + sh(["init", "-q", "-b", "main"]); + sh(["config", "user.email", "a@b"]); + sh(["config", "user.name", "a"]); + await writeFile(path.join(repo, "a.txt"), "one\ntwo\n"); + sh(["add", "a.txt"]); + sh(["commit", "-qm", "init"]); + // Attack: aliases shadowing the exact subcommands codeviewer invokes. + sh(["config", "alias.status", `!${evil} status`]); + sh(["config", "alias.log", `!${evil} log`]); +}); + +afterAll(async () => { + await rm(dir, { recursive: true, force: true }); +}); + +test("a malicious repository cannot define alias.* that runs programs on status()", async () => { + const g = (await Git.open(repo))!; + expect(g).not.toBeNull(); + const s = await g.status(); + // The status call itself must have worked (aliases neutralized, builtin ran). + expect(s.head?.subject).toBe("init"); + expect(existsSync(marker) ? await readFile(marker, "utf8") : "").toBe(""); +}); + +test("a malicious repository cannot define alias.* that runs programs on history()", async () => { + const g = (await Git.open(repo))!; + const h = await g.history("a.txt"); + expect(h!.length).toBeGreaterThan(0); + expect(existsSync(marker) ? await readFile(marker, "utf8") : "").toBe(""); +}); diff --git a/test/http.wsorigin.security.test.ts b/test/http.wsorigin.security.test.ts new file mode 100644 index 0000000..ddf4ae5 --- /dev/null +++ b/test/http.wsorigin.security.test.ts @@ -0,0 +1,68 @@ +import { afterAll, beforeAll, expect, test } from "bun:test"; +import { mkdtemp, rm, writeFile, mkdir, realpath } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import net from "node:net"; +import { startServer } from "../src/server/http.ts"; + +// RED test for the 2026-10 security review: when the server binds a non-loopback +// host, hostOk() (http.ts:115-119) short-circuits to true, so the only DNS-rebinding +// / cross-site-WebSocket guard left is the Origin check at http.ts:266 — which +// compares Origin against the request's own Host header. An attacker page on +// attacker.example that connects to : with Host: attacker.example:port +// sends Origin http://attacker.example:port, the two match, and the upgrade is +// accepted. The Origin check must compare against the server's own origin +// (scheme + bound host + port), not the client-controlled Host. +const token = "S".repeat(43); + +let dir = ""; +let app: ReturnType; +let port = 0; + +/** Raw HTTP/1.1 upgrade request; returns the response status line. */ +const upgrade = (host: string, origin: string | null) => + new Promise((resolve, reject) => { + const sock = net.connect(port, "127.0.0.1", () => { + const lines = [ + "GET /ws/events?t=" + token + " HTTP/1.1", + `Host: ${host}`, + "Upgrade: websocket", + "Connection: Upgrade", + "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==", + "Sec-WebSocket-Version: 13", + ]; + if (origin) lines.push(`Origin: ${origin}`); + sock.write(lines.join("\r\n") + "\r\n\r\n"); + }); + sock.once("data", (d) => { + resolve(d.toString("latin1").split("\r\n")[0]!); + sock.destroy(); + }); + sock.once("error", reject); + setTimeout(() => { sock.destroy(); reject(new Error("timeout")); }, 5000).unref(); + }); + +beforeAll(async () => { + process.env.CODEVIEWER_NO_UPDATE_CHECK = "1"; + dir = await realpath(await mkdtemp(path.join(tmpdir(), "cv-wsorigin-"))); + await mkdir(path.join(dir, "sub")); + await writeFile(path.join(dir, "a.txt"), "line1\n"); + app = startServer({ root: dir, port: 0, host: "0.0.0.0", shell: "/bin/sh", token }); + port = app.server.port!; +}); + +afterAll(async () => { + app.stop(); + await rm(dir, { recursive: true, force: true }); +}); + +test("a WS upgrade whose Origin matches a spoofed Host header is rejected", async () => { + // DNS-rebinding shape: attacker-controlled hostname in both Host and Origin. + const status = await upgrade("attacker.example:1", "http://attacker.example:1"); + expect(status).toContain("403"); +}); + +test("a WS upgrade with the server's real origin is still accepted", async () => { + const status = await upgrade(`127.0.0.1:${port}`, `http://127.0.0.1:${port}`); + expect(status).toContain("101"); +});