diff --git a/.changeset/hardening-symlink-containment.md b/.changeset/hardening-symlink-containment.md new file mode 100644 index 00000000..e35f8e02 --- /dev/null +++ b/.changeset/hardening-symlink-containment.md @@ -0,0 +1,12 @@ +--- +'@moonshot-ai/agent-core-v2': minor +--- + +Re-check file-tool paths against their symlink-resolved target before reading, +writing or editing. Path canonicalization is lexical, so a symlink sitting +inside the workspace reads as inside it while the OS follows the link at open +time. Read, Write and Edit now resolve the longest existing prefix of the path +and require that a path which looked in-workspace is still in-workspace once +symlinks are resolved, and that the resolved target is not a sensitive file even +when the link itself is innocuously named. Paths the caller already gave as +outside the workspace are unaffected — those remain the approval layer's call. diff --git a/packages/agent-core-v2/src/agent/tools/edit/editTool.ts b/packages/agent-core-v2/src/agent/tools/edit/editTool.ts index 44de0ce3..4ae28b37 100644 --- a/packages/agent-core-v2/src/agent/tools/edit/editTool.ts +++ b/packages/agent-core-v2/src/agent/tools/edit/editTool.ts @@ -21,6 +21,7 @@ import { extendWorkspaceWithSkillRoots, + assertRealPathAccess, resolvePathAccessPath, type WorkspaceConfig, } from '#/tool/path-access'; @@ -28,6 +29,7 @@ import { toInputJsonSchema } from '#/tool/input-schema'; import { literalRulePattern, matchesPathRuleSubject } from '#/tool/rule-match'; import { IFileEditService } from '#/app/edit/fileEdit'; import { IHostEnvironment } from '#/os/interface/hostEnvironment'; +import { IHostFileSystem } from '#/os/interface/hostFileSystem'; import { ISessionSkillCatalog } from '#/session/sessionSkillCatalog/skillCatalog'; import { ISessionWorkspaceContext } from '#/session/workspaceContext/workspaceContext'; import { @@ -48,6 +50,7 @@ export class EditTool implements IEditTool { constructor( @IFileEditService private readonly editor: IFileEditService, + @IHostFileSystem private readonly fs: IHostFileSystem, @IHostEnvironment private readonly env: IHostEnvironment, @ISessionWorkspaceContext private readonly workspaceCtx: ISessionWorkspaceContext, @ISessionSkillCatalog private readonly skillCatalog?: ISessionSkillCatalog, @@ -92,6 +95,11 @@ export class EditTool implements IEditTool { } private async execution(args: EditInput, safePath: string): Promise { + // The path was canonicalized lexically; re-check it against what the + // symlinks actually resolve to before editing through them. + await assertRealPathAccess(safePath, args.path, this.workspaceConfig, this.fs, { + pathClass: this.env.pathClass, + }); if (args.old_string === args.new_string) { return { isError: true, diff --git a/packages/agent-core-v2/src/agent/tools/os/read/readTool.ts b/packages/agent-core-v2/src/agent/tools/os/read/readTool.ts index 9d4b5ba4..f027fb79 100644 --- a/packages/agent-core-v2/src/agent/tools/os/read/readTool.ts +++ b/packages/agent-core-v2/src/agent/tools/os/read/readTool.ts @@ -35,6 +35,7 @@ import { import { registerAgentToolService } from '#/agent/toolRegistry/toolContribution'; import { extendWorkspaceWithSkillRoots, + assertRealPathAccess, resolvePathAccessPath, type WorkspaceConfig, } from '#/tool/path-access'; @@ -271,6 +272,11 @@ export class ReadTool implements IReadTool { } private async execution(args: ReadInput, safePath: string): Promise { + // The path was canonicalized lexically; re-check it against what the + // symlinks actually resolve to before reading through them. + await assertRealPathAccess(safePath, args.path, this.workspaceConfig, this.fs, { + pathClass: this.env.pathClass, + }); try { let stat: Awaited>; try { diff --git a/packages/agent-core-v2/src/agent/tools/os/write/writeTool.ts b/packages/agent-core-v2/src/agent/tools/os/write/writeTool.ts index 1f166213..d298f9c1 100644 --- a/packages/agent-core-v2/src/agent/tools/os/write/writeTool.ts +++ b/packages/agent-core-v2/src/agent/tools/os/write/writeTool.ts @@ -32,6 +32,7 @@ import { } from '#/tool/toolContract'; import { registerAgentToolService } from '#/agent/toolRegistry/toolContribution'; import { + assertRealPathAccess, extendWorkspaceWithSkillRoots, resolvePathAccessPath, type WorkspaceConfig, @@ -87,6 +88,11 @@ export class WriteTool implements IWriteTool { } private async execution(args: WriteInput, safePath: string): Promise { + // The path was canonicalized lexically; re-check it against what the + // symlinks actually resolve to before writing through them. + await assertRealPathAccess(safePath, args.path, this.workspaceConfig, this.fs, { + pathClass: this.env.pathClass, + }); const parentError = await this.ensureParentDirectory(safePath); if (parentError !== undefined) { return { isError: true, output: parentError }; diff --git a/packages/agent-core-v2/src/tool/path-access.ts b/packages/agent-core-v2/src/tool/path-access.ts index d5362efc..78ec1204 100644 --- a/packages/agent-core-v2/src/tool/path-access.ts +++ b/packages/agent-core-v2/src/tool/path-access.ts @@ -333,6 +333,103 @@ export function resolvePathAccessPath( }).path; } +export interface PathRealpathResolver { + realpath(path: string): Promise; +} + +export interface AssertRealPathOptions { + readonly pathClass?: PathClass | undefined; + readonly checkSensitive?: boolean | undefined; +} + +/** + * Resolve the longest existing prefix of `abs` through symlinks and re-attach + * the not-yet-existing tail. A write to a new file still gets its parent + * directory resolved, which is where a redirect would sit. + */ +async function realpathExistingPrefix(abs: string, fs: PathRealpathResolver): Promise { + const tail: string[] = []; + let current = abs; + for (let i = 0; i < 256; i++) { + try { + const real = await fs.realpath(current); + return tail.length === 0 ? real : pathe.join(real, ...tail.toReversed()); + } catch { + const parent = pathe.dirname(current); + if (parent === current) return abs; + tail.push(pathe.basename(current)); + current = parent; + } + } + return abs; +} + +async function realWorkspaceRoots( + config: WorkspaceConfig, + fs: PathRealpathResolver, +): Promise { + const roots: string[] = []; + for (const dir of [config.workspaceDir, ...config.additionalDirs]) { + try { + roots.push(await fs.realpath(dir)); + } catch { + roots.push(dir); + } + } + return roots; +} + +/** + * Symlink-aware re-check, run at execution time. + * + * `resolvePathAccess` canonicalizes lexically, so a symlink that sits inside + * the workspace still reads as inside it — while the OS follows the link at + * open time. This re-runs the two checks against the resolved target: + * + * - a path that looked inside the workspace must still be inside it once + * symlinks are resolved (a path the caller already gave as outside is + * governed by the approval layer, so it is left alone here); + * - the resolved target must not be a sensitive file, even when the link + * itself has an innocuous name. + * + * Costs nothing on the common path: when nothing along the path is a symlink + * the resolved path equals the canonical one and this returns immediately. + */ +export async function assertRealPathAccess( + canonicalPath: string, + rawPath: string, + config: WorkspaceConfig, + fs: PathRealpathResolver, + options: AssertRealPathOptions = {}, +): Promise { + const pathClass = options.pathClass ?? DEFAULT_PATH_CLASS; + const checkSensitive = options.checkSensitive ?? DEFAULT_WORKSPACE_ACCESS_POLICY.checkSensitive; + const realPath = await realpathExistingPrefix(canonicalPath, fs); + if (realPath === canonicalPath) return; + + if (checkSensitive && isSensitiveFile(realPath)) { + throw new PathSecurityError( + 'PATH_SENSITIVE', + rawPath, + realPath, + `"${rawPath}" resolves through a symlink to a sensitive file ` + + `(env / credential / SSH key). Access is blocked to protect secrets.`, + ); + } + + if (!isWithinWorkspace(canonicalPath, config, pathClass)) return; + + const roots = await realWorkspaceRoots(config, fs); + if (roots.some((root) => isWithinDirectory(realPath, root, pathClass))) return; + + throw new PathSecurityError( + 'PATH_OUTSIDE_WORKSPACE', + rawPath, + realPath, + `"${rawPath}" resolves through a symlink to "${realPath}", which is outside the workspace.`, + ); +} + export function assertPathAllowed( path: string, cwd: string, diff --git a/packages/agent-core-v2/test/app/edit/tools/edit.test.ts b/packages/agent-core-v2/test/app/edit/tools/edit.test.ts index cd22cb29..09ed6362 100644 --- a/packages/agent-core-v2/test/app/edit/tools/edit.test.ts +++ b/packages/agent-core-v2/test/app/edit/tools/edit.test.ts @@ -75,7 +75,7 @@ function buildTool( reg.define(IFileEditService, FileEditService); }, }); - return new EditTool(ix.get(IFileEditService), env, workspace); + return new EditTool(ix.get(IFileEditService), fs, env, workspace); } function isPromiseLike( diff --git a/packages/agent-core-v2/test/tool/path-access.test.ts b/packages/agent-core-v2/test/tool/path-access.test.ts index 9bc6ed5d..0f50df9c 100644 --- a/packages/agent-core-v2/test/tool/path-access.test.ts +++ b/packages/agent-core-v2/test/tool/path-access.test.ts @@ -1,6 +1,14 @@ +import { mkdir, mkdtemp, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import nodePath from 'node:path'; + import { describe, expect, it } from 'vitest'; -import { extendWorkspaceWithSkillRoots, isSensitiveFile } from '#/tool/path-access'; +import { + assertRealPathAccess, + extendWorkspaceWithSkillRoots, + isSensitiveFile, +} from '#/tool/path-access'; describe('isSensitiveFile', () => { it('flags base .env files in any directory', () => { @@ -102,3 +110,141 @@ describe('extendWorkspaceWithSkillRoots', () => { ).toEqual([]); }); }); + +describe('assertRealPathAccess', () => { + const workspace = { workspaceDir: '/ws', additionalDirs: [] as string[] }; + + /** + * Resolver where `links` maps a path to what it really resolves to, and + * `missing` paths reject the way `realpath` does for a path that does not + * exist yet (which is what makes the guard walk up to the parent). + */ + function resolver( + links: Record, + missing: readonly string[] = [], + ): { realpath: (p: string) => Promise } { + return { + realpath: (p: string) => { + if (missing.includes(p)) return Promise.reject(new Error(`ENOENT: ${p}`)); + for (const [from, to] of Object.entries(links)) { + if (p === from) return Promise.resolve(to); + } + return Promise.resolve(p); + }, + }; + } + + it('allows a path that resolves to itself', async () => { + await expect( + assertRealPathAccess('/ws/src/a.ts', 'src/a.ts', workspace, resolver({}), { + pathClass: 'posix', + }), + ).resolves.toBeUndefined(); + }); + + it('rejects an in-workspace path that resolves outside the workspace', async () => { + // scripts/deploy.sh -> /home/u/.zshrc + await expect( + assertRealPathAccess( + '/ws/scripts/deploy.sh', + 'scripts/deploy.sh', + workspace, + resolver({ '/ws/scripts/deploy.sh': '/home/u/.zshrc' }), + { pathClass: 'posix' }, + ), + ).rejects.toThrow(/outside the workspace/); + }); + + it('rejects a link whose target is sensitive even when the link name is innocuous', async () => { + // notes.md -> /home/u/.aws/credentials + await expect( + assertRealPathAccess( + '/ws/notes.md', + 'notes.md', + workspace, + resolver({ '/ws/notes.md': '/home/u/.aws/credentials' }), + { pathClass: 'posix' }, + ), + ).rejects.toThrow(/sensitive file/); + }); + + it('resolves the parent directory for a file that does not exist yet', async () => { + // scripts -> /etc, so a new file under it lands outside the workspace. + await expect( + assertRealPathAccess( + '/ws/scripts/new.sh', + 'scripts/new.sh', + workspace, + resolver({ '/ws/scripts': '/etc' }, ['/ws/scripts/new.sh']), + { pathClass: 'posix' }, + ), + ).rejects.toThrow(/outside the workspace/); + }); + + it('allows a symlink that stays inside the workspace', async () => { + await expect( + assertRealPathAccess( + '/ws/link.ts', + 'link.ts', + workspace, + resolver({ '/ws/link.ts': '/ws/real.ts' }), + { pathClass: 'posix' }, + ), + ).resolves.toBeUndefined(); + }); + + it('honours additionalDirs as legitimate roots', async () => { + await expect( + assertRealPathAccess( + '/ws/skill', + 'skill', + { workspaceDir: '/ws', additionalDirs: ['/opt/skills'] }, + resolver({ '/ws/skill': '/opt/skills/a' }), + { pathClass: 'posix' }, + ), + ).resolves.toBeUndefined(); + }); + + it('leaves an explicitly-outside path to the approval layer', async () => { + // Already outside the workspace lexically: not this guard's call. + await expect( + assertRealPathAccess( + '/tmp/scratch', + '/tmp/scratch', + workspace, + resolver({ '/tmp/scratch': '/tmp/elsewhere' }), + { pathClass: 'posix' }, + ), + ).resolves.toBeUndefined(); + }); +}); + +describe('assertRealPathAccess against a real filesystem', () => { + it('blocks a real in-workspace symlink that points outside the workspace', async () => { + const hostFs = { realpath: (p: string) => realpath(p) }; + const root = await realpath(await mkdtemp(nodePath.join(tmpdir(), 'kimi-symlink-'))); + try { + const ws = nodePath.join(root, 'repo'); + const outside = nodePath.join(root, 'home'); + await mkdir(nodePath.join(ws, 'scripts'), { recursive: true }); + await mkdir(outside, { recursive: true }); + const target = nodePath.join(outside, '.zshrc'); + await writeFile(target, 'echo hi\n'); + const link = nodePath.join(ws, 'scripts', 'deploy.sh'); + await symlink(target, link); + const workspace = { workspaceDir: ws, additionalDirs: [] as string[] }; + + await expect( + assertRealPathAccess(link, 'scripts/deploy.sh', workspace, hostFs, { pathClass: 'posix' }), + ).rejects.toThrow(/outside the workspace/); + + const real = nodePath.join(ws, 'scripts', 'ok.sh'); + await writeFile(real, '#!/bin/sh\n'); + await expect( + assertRealPathAccess(real, 'scripts/ok.sh', workspace, hostFs, { pathClass: 'posix' }), + ).resolves.toBeUndefined(); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); +});