diff --git a/src/cognition/emergent/SandboxedToolForge.ts b/src/cognition/emergent/SandboxedToolForge.ts index 3731cace1d0..682daa1f1f5 100644 --- a/src/cognition/emergent/SandboxedToolForge.ts +++ b/src/cognition/emergent/SandboxedToolForge.ts @@ -28,12 +28,14 @@ * * Allowlisted APIs (each requires explicit opt-in via {@link SandboxAPI}): * - `fetch` — HTTP requests, domain-restricted via {@link SandboxedToolForgeConfig.fetchDomainAllowlist}. - * - `fs.readFile` — Read-only file access, path-restricted, max 1 MB. + * - `fs.readFile` — Read-only file access, max 1 MB, restricted to the + * configured roots after symlink resolution (a link inside a root cannot + * be used to reach a file outside one). * - `crypto` — Hashing and HMAC only (`createHash`, `createHmac`). */ import { createHash, createHmac, randomUUID } from 'node:crypto'; -import { readFile } from 'node:fs/promises'; +import { readFile, realpath } from 'node:fs/promises'; import * as path from 'node:path'; import type { SandboxExecutionRequest, SandboxExecutionResult, SandboxAPI } from './types.js'; import { CodeSandbox } from '../../safety/sandbox/executor/CodeSandbox.js'; @@ -156,6 +158,21 @@ export class SandboxedToolForge { /** Filesystem roots sandboxed reads may access. */ private readonly fsReadRoots: string[]; + /** + * Real path of each configured root, resolved lazily and cached PER ROOT. + * + * Both sides of a containment check have to be real paths: a configured + * root is frequently itself a link (on macOS `/tmp` is a link to + * `/private/tmp`), so comparing a resolved file against an unresolved root + * would deny perfectly legitimate reads. + * + * Per root, not per set: a set-wide cache that is dropped whenever ANY + * root fails to resolve would re-resolve the roots that DID succeed, and a + * root symlink repointed in between would then be followed to its new + * target — losing the pinning this cache exists to provide. + */ + private readonly realFsReadRootCache = new Map>(); + /** * Hardened node:vm sandbox shared across all execute() calls. Owns the * codeGeneration restriction, frozen console, and explicit-undefined @@ -216,6 +233,36 @@ export class SandboxedToolForge { * the raw message in that case. The hints map to the 4 most common * LLM forge mistakes observed in production. */ + private resolveReadRoots(): Promise { + return Promise.all(this.fsReadRoots.map((root) => this.resolveReadRoot(root))); + } + + /** + * Resolve one configured root, pinning the first success. + * + * A resolved root is pinned for the life of the forge, deliberately: + * re-resolving it per read would let a root symlink be repointed + * underneath a running sandbox, which is the move this containment check + * exists to stop. A caller that needs to follow a retargeted root builds a + * new forge. A FAILED resolution is not pinned, so a root that becomes + * readable later is picked up — and because each root is cached on its + * own, one unreadable root can never un-pin a sibling that resolved. + */ + private resolveReadRoot(root: string): Promise { + const cached = this.realFsReadRootCache.get(root); + if (cached !== undefined) return cached; + + const pending = realpath(root).catch(() => { + // Drop the failure so the next read retries this root. Its lexical + // form never matches a real path, so an unresolvable root cannot + // widen the sandbox while it stays unreadable. + this.realFsReadRootCache.delete(root); + return root; + }); + this.realFsReadRootCache.set(root, pending); + return pending; + } + private describeSyntaxError(message: string): string { const m = message || ''; if (/Unexpected token 'const'/.test(m) || /Unexpected token 'let'/.test(m)) { @@ -475,15 +522,60 @@ export class SandboxedToolForge { extras.fs = { readFile: async (filePath: string) => { const resolvedPath = path.resolve(filePath); - const allowed = this.fsReadRoots.some((root) => { - return resolvedPath === root || resolvedPath.startsWith(`${root}${path.sep}`); - }); - if (!allowed) { + // Containment via path.relative, not a string prefix: a root that + // already ends in a separator (the filesystem root `/`, a Windows + // drive root `C:\`) would otherwise be compared against a doubled + // separator and deny every file beneath it. path.relative also + // applies the platform's own case rules. + const withinRoots = (candidate: string, roots: readonly string[]): boolean => + roots.some((root) => { + const relative = path.relative(root, candidate); + // '' means the candidate IS the root. A relative path that + // climbs out ('..' or '../x') or stays absolute (a different + // Windows drive) is outside it. Checking for the '..' segment + // rather than the '..' prefix keeps a sibling named '..foo' + // from reading as an escape. + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); + }); + + // Lexical pass: rejects the obvious `../../etc/passwd` shape before + // the sandbox pays for any filesystem call. + if (!withinRoots(resolvedPath, this.fsReadRoots)) { throw new Error( `fs.readFile blocked: path "${resolvedPath}" is outside the allowed roots`, ); } - const data = await readFile(filePath); + + // Lexical containment is NOT containment. A symlink sitting inside + // an allowed root resolves to an in-root string while pointing at + // any file on the machine, so string-prefix checking alone reads + // whatever the link targets (CWE-22). Follow the link chain and + // re-check against the roots' own real paths. + // + // The path has already cleared the lexical check, so a resolution + // failure here names something inside an allowed root: surface the + // filesystem's own error (ENOENT and friends) rather than masking + // a missing file as a containment failure. + const realPath = await realpath(resolvedPath); + if (!withinRoots(realPath, await this.resolveReadRoots())) { + throw new Error( + `fs.readFile blocked: path "${resolvedPath}" resolves outside the allowed roots`, + ); + } + + // Read the RESOLVED path, not the caller's string: re-resolving the + // original would walk the link a second time, and the link can be + // repointed between the two walks. (A component of the resolved + // path could still be swapped for a link in that window — closing + // that needs an O_NOFOLLOW handle, which node's fs/promises does + // not expose; it requires local write access inside an allowed + // root, which is already outside this sandbox's threat model.) + const data = await readFile(realPath); if (data.byteLength > 1_048_576) { throw new Error( `fs.readFile blocked: file exceeds 1 MB limit (${data.byteLength} bytes)`, diff --git a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts index 96d245fdb75..208785d2d0a 100644 --- a/src/cognition/emergent/__tests__/sandboxed-forge.test.ts +++ b/src/cognition/emergent/__tests__/sandboxed-forge.test.ts @@ -13,7 +13,10 @@ * 9. Execution time is measured and returned */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, afterAll } from 'vitest'; +import { mkdtempSync, mkdirSync, symlinkSync, writeFileSync, rmSync, realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, parse } from 'node:path'; import { SandboxedToolForge } from '../SandboxedToolForge.js'; import type { SandboxExecutionRequest, SandboxAPI } from '../types.js'; @@ -490,4 +493,154 @@ describe('SandboxedToolForge', () => { // so just assert > 0. expect(result.memoryUsedBytes).toBeGreaterThan(0); }); + + // ------------------------------------------------------------------------- + // fs.readFile containment (CWE-22): a symlink inside an allowed root must + // not reach a file outside one. + // ------------------------------------------------------------------------- + describe('fs.readFile path containment', () => { + // realpathSync: on macOS the OS temp dir is itself a symlink, so the + // fixture paths have to be real before they are handed to the forge as + // roots — otherwise the test would be asserting against the very + // root-resolution behavior it means to exercise. + const base = realpathSync(mkdtempSync(join(tmpdir(), 'forge-fs-'))); + const allowedRoot = join(base, 'allowed'); + const secretDir = join(base, 'secret'); + mkdirSync(allowedRoot, { recursive: true }); + mkdirSync(secretDir, { recursive: true }); + writeFileSync(join(allowedRoot, 'ok.txt'), 'in-root content'); + writeFileSync(join(secretDir, 'secret.txt'), 'SECRET-DO-NOT-READ'); + // The escape: a link that LIVES in the allowed root, so every + // string-prefix check on its own path passes. + symlinkSync(join(secretDir, 'secret.txt'), join(allowedRoot, 'escape.txt')); + + afterAll(() => { + rmSync(base, { recursive: true, force: true }); + }); + + const readRequest = (target: string): SandboxExecutionRequest => + makeRequest( + 'async function execute(input) { return await fs.readFile(input.p); }', + { p: target }, + { allowlist: ['fs.readFile'] }, + ); + + const forgeWithRoot = new SandboxedToolForge({ fsReadRoots: [allowedRoot] }); + + it('reads an ordinary file inside an allowed root', async () => { + const result = await forgeWithRoot.execute(readRequest(join(allowedRoot, 'ok.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('in-root content'); + }); + + it('blocks a symlink that lives inside the root but targets a file outside it', async () => { + const result = await forgeWithRoot.execute(readRequest(join(allowedRoot, 'escape.txt'))); + + expect(result.success).toBe(false); + expect(result.error ?? '').toMatch(/resolves outside the allowed roots/); + // The secret must not leak through the output channel either. + expect(JSON.stringify(result.output ?? null)).not.toMatch(/SECRET-DO-NOT-READ/); + }); + + it('still blocks a plainly out-of-root path before touching the filesystem', async () => { + const result = await forgeWithRoot.execute(readRequest(join(secretDir, 'secret.txt'))); + + expect(result.success).toBe(false); + expect(result.error ?? '').toMatch(/is outside the allowed roots/); + }); + + it('allows reads under a root that ends in a separator (filesystem / drive root)', async () => { + // The prefix form built `//` for a root of `/` and denied everything + // beneath it; path.relative containment has no such edge. + const fsRoot = parse(base).root; + const forgeAtFsRoot = new SandboxedToolForge({ fsReadRoots: [fsRoot] }); + + const result = await forgeAtFsRoot.execute(readRequest(join(allowedRoot, 'ok.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('in-root content'); + }); + + it('does not treat a sibling whose name merely starts with ".." as an escape', async () => { + const oddSibling = join(base, '..odd'); + mkdirSync(oddSibling, { recursive: true }); + writeFileSync(join(oddSibling, 'f.txt'), 'sibling content'); + const forgeAtBase = new SandboxedToolForge({ fsReadRoots: [base] }); + + const result = await forgeAtBase.execute(readRequest(join(oddSibling, 'f.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('sibling content'); + }); + + it('pins a resolved root, so repointing the link mid-life cannot move the sandbox', async () => { + // Deliberate: re-resolving the root per read would let an attacker who + // can rewrite the link relocate a running sandbox. The first read fixes + // the real root; the retargeted directory is then outside it. + const movingRoot = join(base, 'moving'); + const firstTarget = join(base, 'target-a'); + const secondTarget = join(base, 'target-b'); + mkdirSync(firstTarget, { recursive: true }); + mkdirSync(secondTarget, { recursive: true }); + writeFileSync(join(firstTarget, 'f.txt'), 'target A'); + writeFileSync(join(secondTarget, 'f.txt'), 'target B'); + symlinkSync(firstTarget, movingRoot); + + const forgeMoving = new SandboxedToolForge({ fsReadRoots: [movingRoot] }); + const first = await forgeMoving.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(first.success).toBe(true); + expect(first.output).toBe('target A'); + + rmSync(movingRoot); + symlinkSync(secondTarget, movingRoot); + + const second = await forgeMoving.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(second.success).toBe(false); + expect(second.error ?? '').toMatch(/resolves outside the allowed roots/); + // A forge built after the change follows the link's new target. + const freshForge = new SandboxedToolForge({ fsReadRoots: [movingRoot] }); + const fresh = await freshForge.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(fresh.success).toBe(true); + expect(fresh.output).toBe('target B'); + }); + + it('keeps a resolved root pinned even when a sibling root never resolves', async () => { + // A set-wide cache dropped on any failure would re-resolve the symlink + // root on the next read and follow it to its new target. + const movingRoot = join(base, 'moving-mixed'); + const firstTarget = join(base, 'mixed-a'); + const secondTarget = join(base, 'mixed-b'); + mkdirSync(firstTarget, { recursive: true }); + mkdirSync(secondTarget, { recursive: true }); + writeFileSync(join(firstTarget, 'f.txt'), 'mixed A'); + writeFileSync(join(secondTarget, 'f.txt'), 'mixed B'); + symlinkSync(firstTarget, movingRoot); + + const forgeMixed = new SandboxedToolForge({ + fsReadRoots: [movingRoot, join(base, 'never-exists')], + }); + const first = await forgeMixed.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(first.success).toBe(true); + expect(first.output).toBe('mixed A'); + + rmSync(movingRoot); + symlinkSync(secondTarget, movingRoot); + + const second = await forgeMixed.execute(readRequest(join(movingRoot, 'f.txt'))); + expect(second.success).toBe(false); + expect(second.error ?? '').toMatch(/resolves outside the allowed roots/); + }); + + it('allows a root that is itself a symlink (both sides are resolved)', async () => { + const linkedRoot = join(base, 'linked-root'); + symlinkSync(allowedRoot, linkedRoot); + const forgeViaLink = new SandboxedToolForge({ fsReadRoots: [linkedRoot] }); + + const result = await forgeViaLink.execute(readRequest(join(linkedRoot, 'ok.txt'))); + + expect(result.success).toBe(true); + expect(result.output).toBe('in-root content'); + }); + }); });