diff --git a/CHANGELOG.md b/CHANGELOG.md index 456764a2eee..c9d8340feb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,7 @@ Docs: https://docs.openclaw.ai - Feishu/Security: harden media URL fetching against SSRF and local file disclosure. (#16285) Thanks @mbelinky. - Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. - Security/Agents: enforce workspace-root path bounds for `apply_patch` in non-sandbox mode to block traversal and symlink escape writes. Thanks @p80n-sec. +- Security/Agents: enforce symlink-escape checks for `apply_patch` delete hunks under `workspaceOnly`, while still allowing deleting the symlink itself. Thanks @p80n-sec. - Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent. - Security: fix Chutes manual OAuth login state validation by requiring the full redirect URL (reject code-only pastes) (thanks @aether-ai-agent). - Security/Gateway: harden tool-supplied `gatewayUrl` overrides by restricting them to loopback or the configured `gateway.remote.url`. Thanks @p80n-sec. diff --git a/src/agents/apply-patch.e2e.test.ts b/src/agents/apply-patch.e2e.test.ts index c3d3fa9ace2..44082058a34 100644 --- a/src/agents/apply-patch.e2e.test.ts +++ b/src/agents/apply-patch.e2e.test.ts @@ -181,9 +181,7 @@ describe("applyPatch", () => { *** End Patch`; try { - await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow( - /Symlink escapes sandbox root/, - ); + await expect(applyPatch(patch, { cwd: dir })).rejects.toThrow(/Symlink escapes sandbox root/); const stillThere = await fs.readFile(outsideFile, "utf8"); expect(stillThere).toBe("victim\n"); } finally { @@ -216,4 +214,29 @@ describe("applyPatch", () => { } }); }); + + it("allows deleting a symlink itself even if it points outside cwd", async () => { + await withTempDir(async (dir) => { + const outsideDir = await fs.mkdtemp(path.join(path.dirname(dir), "openclaw-patch-outside-")); + try { + const outsideTarget = path.join(outsideDir, "target.txt"); + await fs.writeFile(outsideTarget, "keep\n", "utf8"); + + const linkDir = path.join(dir, "link"); + await fs.symlink(outsideDir, linkDir); + + const patch = `*** Begin Patch +*** Delete File: link +*** End Patch`; + + const result = await applyPatch(patch, { cwd: dir }); + expect(result.summary.deleted).toEqual(["link"]); + await expect(fs.lstat(linkDir)).rejects.toBeDefined(); + const outsideContents = await fs.readFile(outsideTarget, "utf8"); + expect(outsideContents).toBe("keep\n"); + } finally { + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); + }); }); diff --git a/src/agents/apply-patch.ts b/src/agents/apply-patch.ts index 32c017e495a..76ddc1a3dd0 100644 --- a/src/agents/apply-patch.ts +++ b/src/agents/apply-patch.ts @@ -155,7 +155,7 @@ export async function applyPatch( } if (hunk.kind === "delete") { - const target = await resolvePatchPath(hunk.path, options); + const target = await resolvePatchPath(hunk.path, options, "unlink"); await fileOps.remove(target.resolved); recordSummary(summary, seen, "deleted", target.display); continue; @@ -254,6 +254,7 @@ async function ensureDir(filePath: string, ops: PatchFileOps) { async function resolvePatchPath( filePath: string, options: ApplyPatchOptions, + purpose: "readWrite" | "unlink" = "readWrite", ): Promise<{ resolved: string; display: string }> { if (options.sandbox) { const resolved = options.sandbox.bridge.resolvePath({ @@ -273,6 +274,7 @@ async function resolvePatchPath( filePath, cwd: options.cwd, root: options.cwd, + allowFinalSymlink: purpose === "unlink", }) ).resolved : resolvePathFromCwd(filePath, options.cwd); diff --git a/src/agents/sandbox-paths.ts b/src/agents/sandbox-paths.ts index 176854b0073..c7a5192bc53 100644 --- a/src/agents/sandbox-paths.ts +++ b/src/agents/sandbox-paths.ts @@ -50,9 +50,16 @@ export function resolveSandboxPath(params: { filePath: string; cwd: string; root return { resolved, relative }; } -export async function assertSandboxPath(params: { filePath: string; cwd: string; root: string }) { +export async function assertSandboxPath(params: { + filePath: string; + cwd: string; + root: string; + allowFinalSymlink?: boolean; +}) { const resolved = resolveSandboxPath(params); - await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root)); + await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root), { + allowFinalSymlink: params.allowFinalSymlink, + }); return resolved; } @@ -90,18 +97,29 @@ export async function resolveSandboxedMediaSource(params: { return resolved.resolved; } -async function assertNoSymlinkEscape(relative: string, root: string) { +async function assertNoSymlinkEscape( + relative: string, + root: string, + options?: { allowFinalSymlink?: boolean }, +) { if (!relative) { return; } const rootReal = await tryRealpath(root); const parts = relative.split(path.sep).filter(Boolean); let current = root; - for (const part of parts) { + for (let idx = 0; idx < parts.length; idx += 1) { + const part = parts[idx]; + const isLast = idx === parts.length - 1; current = path.join(current, part); try { const stat = await fs.lstat(current); if (stat.isSymbolicLink()) { + // Unlinking a symlink itself is safe even if it points outside the root. What we + // must prevent is traversing through a symlink to reach targets outside root. + if (options?.allowFinalSymlink && isLast) { + return; + } const target = await tryRealpath(current); if (!isPathInside(rootReal, target)) { throw new Error(