diff --git a/CHANGELOG.md b/CHANGELOG.md index 4659b05..2f39d67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,17 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ### Fixed +- **A Bot's shell was handed the deployment's secrets.** A command ran with the whole environment of + the process that started it, and in the one-container image that environment carries + `COMPUTER_TOKEN`, `DATABASE_URL`, `KEY_ENCRYPTION_KEY`, the licence and the model key. The token is + the only thing in front of the computer's control surface, and the shell runs inside the process + that serves it, so a command could drive the browser over loopback with no policy decision and no + audit row: the gateway was optional for the one actor it exists to constrain. The database URL and + the encryption key together opened the credential vault and the trail meant to record what + happened. A command now receives `HOME`, `PATH`, `DEBIAN_FRONTEND` and the locale, and nothing + else. `apt-get` also stops waiting for a prompt nobody can answer. + + - **A deployment served over plain HTTP could not start a conversation.** The chat surface minted identifiers with `crypto.randomUUID`, which browsers withhold outside a secure context. On a laptop `http://localhost` counts as one, so this never showed up in development; on a real diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 40e487f..a2cbb21 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -19,6 +19,43 @@ import { spawn } from "node:child_process"; * between Bots, a shell is shared too. */ +/** + * The environment a Bot's command is allowed to see. + * + * An allow-list rather than a deny-list, because the interesting names are the ones nobody thought + * of. This process is started with the container's whole environment on purpose: it needs + * `COMPUTER_TOKEN` to authenticate its own callers. In the one-container image that environment also + * carries `DATABASE_URL`, `KEY_ENCRYPTION_KEY`, the licence and the model key, because there is one + * environment and every service reads it. + * + * Handing that to a command a Bot wrote would undo the gateway. `COMPUTER_TOKEN` is the only thing in + * front of the control surface on port 4100, and this shell runs inside the process that serves it, + * so a Bot holding the token could drive the browser over loopback with no policy decision and no + * audit row. The rest is worse in a quieter way: the database URL and the encryption key together + * open the credential vault and the trail that is supposed to record what happened. + * + * What is left is what a command needs to run at all. Without `PATH` nothing resolves, and the tool + * description tells the model to install packages, which needs one. + */ +const INHERITED = ["PATH", "TERM", "LANG", "LC_ALL", "TZ"] as const; + +function commandEnvironment(workspaceDir: string): NodeJS.ProcessEnv { + const environment: NodeJS.ProcessEnv = { + // The workspace, so a relative path means the same thing here as it does to the file tools. + HOME: workspaceDir, + // Fallback for a container that sets no PATH of its own; an inherited one overwrites it below. + PATH: "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + // `apt-get install` without this waits for an answer nobody is there to give, and the command + // times out looking like a broken package rather than a prompt. + DEBIAN_FRONTEND: "noninteractive", + }; + for (const name of INHERITED) { + const value = process.env[name]; + if (value !== undefined) environment[name] = value; + } + return environment; +} + /** Long enough for an install, short enough that a hung command is not a hung Bot. */ const DEFAULT_TIMEOUT_MS = 120_000; const MAX_TIMEOUT_MS = 600_000; @@ -78,7 +115,9 @@ export function createShell(workspaceDir: string) { */ const child = spawn("/bin/bash", ["-lc", input.command], { cwd: workspaceDir, - env: { ...process.env, HOME: workspaceDir }, + // Not `process.env`. See INHERITED above: this process holds the deployment's secrets and a + // command a Bot wrote is the last thing that should be able to read them. + env: commandEnvironment(workspaceDir), }); let stdout = ""; diff --git a/agent-computer/tests/shell-environment.test.ts b/agent-computer/tests/shell-environment.test.ts new file mode 100644 index 0000000..534e0fe --- /dev/null +++ b/agent-computer/tests/shell-environment.test.ts @@ -0,0 +1,98 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createShell } from "../src/shell"; + +/** + * What a Bot's command can read out of the process that runs it. + * + * This process is started with the container's whole environment, deliberately: it needs + * `COMPUTER_TOKEN` to authenticate its own callers. In the one-container image that environment also + * carries `DATABASE_URL`, `KEY_ENCRYPTION_KEY` and the licence, because there is one environment and + * every service reads it. + * + * A command a Bot wrote must not see any of that. `COMPUTER_TOKEN` is the sharpest case: it is the + * only thing standing in front of the control surface on port 4100, and this shell runs inside the + * process that serves it. A Bot holding that token could drive the browser over loopback, which skips + * the policy decision and the audit row that are the entire point of the gateway. + * + * The assertion is a whole-set one rather than a list of names to exclude, because the dangerous name + * is the one nobody thought of. A variable added to a deployment next year is covered by this without + * anybody remembering to add it here. + * + * Against a real shell, not a mock: the question is what a process actually inherits, and a mock + * would only answer what we believed it inherits. The marker below uses a name nothing else reads, + * because a test that sets `DATABASE_URL` on the real environment breaks every other test that + * reads one. + */ + +const ALLOWED = new Set([ + "HOME", + "PATH", + "DEBIAN_FRONTEND", + "TERM", + "LANG", + "LC_ALL", + "TZ", + // bash sets these itself for any shell it starts; they are not inherited from this process. + "PWD", + "SHLVL", + "_", + "OLDPWD", + "SHELL", +]); + +const MARKER = "OPENBOT_SHELL_ENV_TEST_MARKER"; +const MARKER_VALUE = "this-must-not-reach-a-bot-command"; + +let workspace: string; + +beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), "openbot-shell-env-")); + process.env[MARKER] = MARKER_VALUE; +}); + +afterEach(async () => { + delete process.env[MARKER]; + await rm(workspace, { recursive: true, force: true }); +}); + +describe("a command a Bot wrote", () => { + test("sees only the variables it was explicitly given", async () => { + const result = await createShell(workspace).run({ + // One name per line, values dropped: the question is which variables exist at all. + command: "env | cut -d= -f1 | sort -u", + }); + + expect(result.exitCode).toBe(0); + const leaked = result.stdout + .split("\n") + .map((line) => line.trim()) + .filter((name) => name.length > 0 && !ALLOWED.has(name)); + + expect(leaked).toEqual([]); + }); + + test("cannot read a secret held by the process that runs it", async () => { + const result = await createShell(workspace).run({ + command: `echo "[$${MARKER}]"`, + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe("[]"); + expect(result.stdout).not.toContain(MARKER_VALUE); + }); + + test("still has the environment a command needs to work", async () => { + // The point is an allow-list, not an empty environment. Without PATH nothing resolves, and the + // tool description tells the model to install packages, which needs one. + const result = await createShell(workspace).run({ + command: 'echo "$HOME"; command -v sh >/dev/null && echo "path works"', + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain(workspace); + expect(result.stdout).toContain("path works"); + }); +});