Skip to content

Commit fec4b71

Browse files
authored
Check WS Origin against the server's own origins, not the Host header (#16)
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.
1 parent 7b4957a commit fec4b71

4 files changed

Lines changed: 160 additions & 1 deletion

File tree

‎DESIGN.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -874,3 +874,15 @@ The Linux app was not affected by the image bug: its Chromium is confined with
874874
`--host-resolver-rules` and an unreachable `--proxy-server`, neither of which applies to `blob:`
875875
URLs, which never reach the network stack. There, TIFF and HEIC (which Chromium cannot decode) now
876876
open as the hex dump under a notice.
877+
878+
## Security review, October 2026
879+
880+
An adversarial review (three parallel reviewers plus RED-test validation of the top
881+
findings) re-walked the server surface. One finding was validated and fixed; several
882+
were refuted or accepted as trade-offs.
883+
884+
| Finding | Fix |
885+
| --- | --- |
886+
| 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. |
887+
| (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. |
888+
| (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. |

‎src/server/http.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { Server, ServerWebSocket } from "bun";
22
import { randomBytes, timingSafeEqual } from "node:crypto";
33
import path from "node:path";
4+
import os from "node:os";
45
import { realpath, stat } from "node:fs/promises";
56
import { listDir, readForView, resolveSafe, imageMime, toRel, looksBinary, openRegular, MAX_TEXT_BYTES } from "./files.ts";
67
import { FileIndex } from "./index.ts";
@@ -66,6 +67,23 @@ export function startServer(opts: ServeOptions): { server: Server<SocketData>; u
6667
let port = 0;
6768
const loopback = ["127.0.0.1", "localhost", "::1"].includes(opts.host);
6869

70+
/**
71+
* Every origin a legitimate browser page can have when talking to this server:
72+
* loopback names plus every local interface address (a non-loopback --host can be
73+
* reached under any of them). Never derived from the request's Host header, which
74+
* the client controls — matching Origin against it made the cross-site WebSocket
75+
* guard a no-op under DNS rebinding.
76+
*/
77+
const selfOrigins = (): Set<string> => {
78+
const set = new Set<string>([`http://localhost:${port}`, `http://127.0.0.1:${port}`, `http://[::1]:${port}`]);
79+
if (!loopback) {
80+
for (const addrs of Object.values(os.networkInterfaces())) {
81+
for (const a of addrs ?? []) set.add(`http://${a.address.includes(":") ? `[${a.address}]` : a.address}:${port}`);
82+
}
83+
}
84+
return set;
85+
};
86+
6987
const broadcast = (msg: unknown) => {
7088
const s = JSON.stringify(msg);
7189
for (const ws of events) ws.send(s);
@@ -262,8 +280,9 @@ export function startServer(opts: ServeOptions): { server: Server<SocketData>; u
262280

263281
if (p.startsWith("/ws/")) {
264282
// Cross-site WebSocket hijacking guard: browsers always send Origin; it must be us.
283+
// Compared against this server's own origins, never the client-controlled Host.
265284
const o = req.headers.get("origin");
266-
if (o && o !== `http://${req.headers.get("host")}`) return fail(403, "bad origin");
285+
if (o && !selfOrigins().has(o)) return fail(403, "bad origin");
267286
if (p === "/ws/events") {
268287
return srv.upgrade(req, { data: { kind: "events" } }) ? undefined : fail(400, "upgrade failed");
269288
}

‎test/git.alias.security.test.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import { afterAll, beforeAll, expect, test } from "bun:test";
2+
import { mkdtemp, rm, writeFile, mkdir, readFile, chmod, realpath } from "node:fs/promises";
3+
import { existsSync } from "node:fs";
4+
import { tmpdir } from "node:os";
5+
import path from "node:path";
6+
import { Git } from "../src/server/git.ts";
7+
8+
// RED tests for the 2026-10 security review: a repository whose .git/config defines
9+
// alias.* entries must not be able to make codeviewer's git calls run programs.
10+
// git.ts's neutralize() only blanks filter./diff./merge. drivers (git.ts:191);
11+
// alias.* is a separate config family that expands a shell command when the invoked
12+
// subcommand name matches (e.g. `git status`, `git log` as called at git.ts:242-244).
13+
let dir = "";
14+
let repo = "";
15+
let marker = "";
16+
17+
const sh = (cmd: string[], cwd = repo) => {
18+
const r = Bun.spawnSync(["git", ...cmd], { cwd, stderr: "pipe" });
19+
if (r.exitCode !== 0) throw new Error(`git ${cmd.join(" ")}: ${r.stderr}`);
20+
return r.stdout.toString();
21+
};
22+
23+
beforeAll(async () => {
24+
dir = await realpath(await mkdtemp(path.join(tmpdir(), "cv-gitalias-")));
25+
repo = path.join(dir, "repo");
26+
marker = path.join(dir, "pwned.log");
27+
await mkdir(repo);
28+
const evil = path.join(dir, "evil.sh");
29+
await writeFile(evil, `#!/bin/sh\necho "ran $*" >> '${marker}'\n`);
30+
await chmod(evil, 0o755);
31+
sh(["init", "-q", "-b", "main"]);
32+
sh(["config", "user.email", "a@b"]);
33+
sh(["config", "user.name", "a"]);
34+
await writeFile(path.join(repo, "a.txt"), "one\ntwo\n");
35+
sh(["add", "a.txt"]);
36+
sh(["commit", "-qm", "init"]);
37+
// Attack: aliases shadowing the exact subcommands codeviewer invokes.
38+
sh(["config", "alias.status", `!${evil} status`]);
39+
sh(["config", "alias.log", `!${evil} log`]);
40+
});
41+
42+
afterAll(async () => {
43+
await rm(dir, { recursive: true, force: true });
44+
});
45+
46+
test("a malicious repository cannot define alias.* that runs programs on status()", async () => {
47+
const g = (await Git.open(repo))!;
48+
expect(g).not.toBeNull();
49+
const s = await g.status();
50+
// The status call itself must have worked (aliases neutralized, builtin ran).
51+
expect(s.head?.subject).toBe("init");
52+
expect(existsSync(marker) ? await readFile(marker, "utf8") : "").toBe("");
53+
});
54+
55+
test("a malicious repository cannot define alias.* that runs programs on history()", async () => {
56+
const g = (await Git.open(repo))!;
57+
const h = await g.history("a.txt");
58+
expect(h!.length).toBeGreaterThan(0);
59+
expect(existsSync(marker) ? await readFile(marker, "utf8") : "").toBe("");
60+
});
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import { afterAll, beforeAll, expect, test } from "bun:test";
2+
import { mkdtemp, rm, writeFile, mkdir, realpath } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import path from "node:path";
5+
import net from "node:net";
6+
import { startServer } from "../src/server/http.ts";
7+
8+
// RED test for the 2026-10 security review: when the server binds a non-loopback
9+
// host, hostOk() (http.ts:115-119) short-circuits to true, so the only DNS-rebinding
10+
// / cross-site-WebSocket guard left is the Origin check at http.ts:266 — which
11+
// compares Origin against the request's own Host header. An attacker page on
12+
// attacker.example that connects to <lan-ip>:<port> with Host: attacker.example:port
13+
// sends Origin http://attacker.example:port, the two match, and the upgrade is
14+
// accepted. The Origin check must compare against the server's own origin
15+
// (scheme + bound host + port), not the client-controlled Host.
16+
const token = "S".repeat(43);
17+
18+
let dir = "";
19+
let app: ReturnType<typeof startServer>;
20+
let port = 0;
21+
22+
/** Raw HTTP/1.1 upgrade request; returns the response status line. */
23+
const upgrade = (host: string, origin: string | null) =>
24+
new Promise<string>((resolve, reject) => {
25+
const sock = net.connect(port, "127.0.0.1", () => {
26+
const lines = [
27+
"GET /ws/events?t=" + token + " HTTP/1.1",
28+
`Host: ${host}`,
29+
"Upgrade: websocket",
30+
"Connection: Upgrade",
31+
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==",
32+
"Sec-WebSocket-Version: 13",
33+
];
34+
if (origin) lines.push(`Origin: ${origin}`);
35+
sock.write(lines.join("\r\n") + "\r\n\r\n");
36+
});
37+
sock.once("data", (d) => {
38+
resolve(d.toString("latin1").split("\r\n")[0]!);
39+
sock.destroy();
40+
});
41+
sock.once("error", reject);
42+
setTimeout(() => { sock.destroy(); reject(new Error("timeout")); }, 5000).unref();
43+
});
44+
45+
beforeAll(async () => {
46+
process.env.CODEVIEWER_NO_UPDATE_CHECK = "1";
47+
dir = await realpath(await mkdtemp(path.join(tmpdir(), "cv-wsorigin-")));
48+
await mkdir(path.join(dir, "sub"));
49+
await writeFile(path.join(dir, "a.txt"), "line1\n");
50+
app = startServer({ root: dir, port: 0, host: "0.0.0.0", shell: "/bin/sh", token });
51+
port = app.server.port!;
52+
});
53+
54+
afterAll(async () => {
55+
app.stop();
56+
await rm(dir, { recursive: true, force: true });
57+
});
58+
59+
test("a WS upgrade whose Origin matches a spoofed Host header is rejected", async () => {
60+
// DNS-rebinding shape: attacker-controlled hostname in both Host and Origin.
61+
const status = await upgrade("attacker.example:1", "http://attacker.example:1");
62+
expect(status).toContain("403");
63+
});
64+
65+
test("a WS upgrade with the server's real origin is still accepted", async () => {
66+
const status = await upgrade(`127.0.0.1:${port}`, `http://127.0.0.1:${port}`);
67+
expect(status).toContain("101");
68+
});

0 commit comments

Comments
 (0)