diff --git a/CHANGELOG.md b/CHANGELOG.md index 33b92b1..5375f33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,6 +159,13 @@ Sessions survive and nobody signs in again. access and refresh tokens use Better Auth's own encryption, keyed on `BETTER_AUTH_SECRET`. - **A failed provider registration looked like a button that did not work.** The error was rendered on the page behind the dialog, which was covering it. +- **A write could follow a symlink out of the Bot's workspace.** The confinement resolved the + directory a write would land in but not the name it would land on, so a link left at `notes.txt` + pointing outside was followed by the write; a read through the identical link was already refused. + The gateway had already decided and written the audit row against the path as it was asked for, so a + rule written for `credentials/` never saw the file that was written and the trail named a file + nothing had touched. A dangling link escaped the same way, because resolving the path throws where + the write would still land. Links pointing back inside the workspace continue to work. - **A Bot could become root inside its container.** `sudo` was granted as `NOPASSWD: ALL`, and the comment above it named the two conditions that made that acceptable: the container being one Bot's alone, and not holding a database. The image meets neither, because the supervisor is deliberately diff --git a/agent-computer/src/workspace.ts b/agent-computer/src/workspace.ts index 4e06f09..760de05 100644 --- a/agent-computer/src/workspace.ts +++ b/agent-computer/src/workspace.ts @@ -14,16 +14,19 @@ * 3. The resolved path must still be inside the root after symlinks are followed. This is the layer * people miss: a symlink placed inside the workspace (by an earlier write, or by a page the Bot * downloaded something from) passes the lexical check and then points anywhere on the filesystem. - * For a write, the file may not exist yet, so it is the deepest existing ancestor that gets - * resolved, which is the directory the write will actually land in. + * For a write, the file may not exist yet, so the deepest existing ancestor gets resolved, which + * is the directory the write will land in, AND the name itself is resolved when something is + * already there, because `writeFile` follows a link at the last component too. * * A factory taking its root as an argument rather than reading the environment, so the confinement * can be tested against a temporary directory instead of being taken on trust. */ import { + lstat, mkdir, readdir, readFile, + readlink, realpath, stat, writeFile, @@ -131,10 +134,42 @@ export function createWorkspace( } assertInside(root, realAnchor, wanted); - // For a write, return the full lexical target. It is already proven contained lexically, and the - // deepest existing directory is proven contained after symlinks, so `mkdir -p` can only create the - // rest inside the workspace. - return forWrite ? target : realAnchor; + if (!forWrite) return realAnchor; + + /* + * Layer three again, for the last component rather than the directory holding it. + * + * Containing `dirname(target)` proves where a NEW file would be created. It proves nothing about + * a name that already exists, and `writeFile` follows a symlink at the last component the same + * way `readFile` does. A link at `notes.txt` pointing at `/root/.ssh/authorized_keys` passes + * every check above, having no `..`, not being absolute, and sitting directly in the workspace, + * and the bytes land outside the volume. The read side already refuses the identical link; the + * write side was the asymmetry. + * + * The link has to get there first, which takes a shell or an archive that was unpacked with one, + * so this is not a fresh escape for a Bot that already has `run_command`: that Bot can write + * outside directly. What it is, is a hole in what the gateway can still see. The decision and the + * audit row are both made against the path as the Bot asked for it, so a rule written for + * `credentials/` or `*.env` is evaluated against `notes.txt` and never sees the file that gets + * written, and the row names a file in the workspace that nothing touched. A deployment that + * denies `run_command` and allows writes is relying on exactly that, and so is one reading the + * trail afterwards. A permissive workspace is a decision a deployment can make. A trail that + * describes a different file from the one on disk is not. + */ + const landing = await writeDestination(target, wanted); + if (landing === target) return target; + + // A link was followed, so the destination gets the checks the requested path already passed: + // inside lexically, and inside after the directory holding it is resolved. + assertInside(root, landing, wanted); + const holder = await realpath(dirname(landing)).catch(() => null); + if (holder === null) { + throw new WorkspacePathError( + `${wanted} points at somewhere that does not exist, so where a write would land cannot be established.`, + ); + } + assertInside(root, holder, wanted); + return landing; } return { @@ -277,6 +312,45 @@ function assertInside(root: string, candidate: string, shown?: string): void { } } +/** + * How many links a chain may pass through before it is treated as a cycle rather than a path. + * + * Linux gives up at 40. Anything approaching this is a loop or a deliberate attempt to make the walk + * expensive, and neither is a file a Bot needs to write. + */ +const MAX_LINK_HOPS = 32; + +/** + * Where a write to `target` would actually put the bytes. + * + * Returns `target` unchanged when nothing is there or what is there is not a link, which is every + * ordinary write. Only a name that is already a symlink walks. + * + * Walked with `lstat` and `readlink` rather than resolved with `realpath`, because `realpath` throws + * on a DANGLING link and `writeFile` creates the file at its destination regardless. A link aimed at + * a name that does not exist yet would otherwise escape through the failure path rather than the + * success one, which is the harder version of the bug to notice. + * + * Confining rather than forbidding, the same as the read side. A link that points back inside the + * workspace keeps working: refusing every link would be easier and would break legitimate use. + */ +async function writeDestination( + target: string, + shown: string, +): Promise { + let current = target; + for (let hop = 0; hop <= MAX_LINK_HOPS; hop += 1) { + const entry = await lstat(current).catch(() => null); + // Nothing there, or something that is not a link. This is where the write lands. + if (entry === null || !entry.isSymbolicLink()) return current; + // A relative link is relative to the directory the link sits in, not to the workspace root. + current = resolve(dirname(current), await readlink(current)); + } + throw new WorkspacePathError( + `${shown} is a chain of links that does not settle, so where a write would land cannot be established.`, + ); +} + /** The closest ancestor of `target` that exists, never above `root`. */ async function nearestExistingAncestor( root: string, diff --git a/agent-computer/tests/workspace.test.ts b/agent-computer/tests/workspace.test.ts index 47eb97f..f95001e 100644 --- a/agent-computer/tests/workspace.test.ts +++ b/agent-computer/tests/workspace.test.ts @@ -1,5 +1,12 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; +import { + mkdir, + mkdtemp, + readFile, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -206,6 +213,74 @@ describe("escaping the workspace", () => { ); }); + test("refuses to write THROUGH a symlinked FILE that points outside", async () => { + // The asymmetry between the two tests above. Resolving `dirname` catches a link standing in for a + // directory; a link standing in for the FILE has the workspace as its parent and passes, and then + // `writeFile` follows it. Refusing is only half of what this asserts: the file outside has to be + // untouched afterwards, because an error thrown after the bytes landed would still be an escape. + const secret = join(outside, "secret.txt"); + await symlink(secret, join(root, "notes.txt")); + await expect(workspace().write("notes.txt", "owned")).rejects.toThrow( + WorkspacePathError, + ); + expect(await readFile(secret, "utf8")).toBe("a private key"); + }); + + test("refuses to append THROUGH a symlinked file that points outside", async () => { + // `append` is a separate flag reaching a separate `writeFile` mode, so it is a separate way in. + const secret = join(outside, "secret.txt"); + await symlink(secret, join(root, "log.txt")); + await expect( + workspace().write("log.txt", "owned", { append: true }), + ).rejects.toThrow(WorkspacePathError); + expect(await readFile(secret, "utf8")).toBe("a private key"); + }); + + test("refuses to write through a DANGLING link that points outside", async () => { + // The harder half. `realpath` throws on a link whose destination does not exist, so a check built + // on it treats this as "no such file" and lets the write through the failure path, while + // `writeFile` creates the destination regardless. Nothing exists here to prove the escape with, + // so the assertion is that the file was never created outside. + const notThere = join(outside, "planted.txt"); + await symlink(notThere, join(root, "fresh.txt")); + await expect(workspace().write("fresh.txt", "owned")).rejects.toThrow( + WorkspacePathError, + ); + await expect(readFile(notThere, "utf8")).rejects.toThrow(); + }); + + test("refuses a chain of links that ends up outside", async () => { + // One hop is the obvious case and the only one a single `readlink` would catch. + await symlink(join(outside, "secret.txt"), join(root, "second.txt")); + await symlink(join(root, "second.txt"), join(root, "first.txt")); + await expect(workspace().write("first.txt", "owned")).rejects.toThrow( + WorkspacePathError, + ); + expect(await readFile(join(outside, "secret.txt"), "utf8")).toBe( + "a private key", + ); + }); + + test("refuses a cycle of links rather than following it forever", async () => { + // Two links pointing at each other never reach something that is not a link. The walk has to stop + // on its own and say why, instead of spinning or surfacing an ELOOP from the write. + await symlink(join(root, "b.txt"), join(root, "a.txt")); + await symlink(join(root, "a.txt"), join(root, "b.txt")); + await expect(workspace().write("a.txt", "owned")).rejects.toThrow( + WorkspacePathError, + ); + }); + + test("writing THROUGH a link that points back inside still works", async () => { + // Confining, not forbidding, on the write side too. Refusing every link would pass the tests + // above and quietly break a Bot that keeps `latest.csv` pointing at the newest report. + const ws = workspace(); + await ws.write("real/data.txt", "before"); + await symlink(join(root, "real/data.txt"), join(root, "alias.txt")); + await ws.write("alias.txt", "after"); + expect((await ws.read("real/data.txt")).text).toBe("after"); + }); + test("a symlink pointing back INSIDE the workspace still works", async () => { // The guard must confine, not merely forbid symlinks: refusing every link would be easier and // would break legitimate use.