From 3eb6a31b6fcf8268456988bfa8e3637d373438c2 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 13 Feb 2026 01:24:46 +0100 Subject: [PATCH] fix: confine sandbox skill sync destinations --- CHANGELOG.md | 1 + ...erged-skills-into-target-workspace.test.ts | 66 +++++++++++++++++++ src/agents/skills/workspace.ts | 62 ++++++++++++++++- 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80b1f9e12..a8e6b5d9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai - Security: fix unauthenticated Nostr profile API remote config tampering. (#13719) Thanks @coygeek. - Security: remove bundled soul-evil hook. (#14757) Thanks @Imccccc. +- Security/Sandbox: confine mirrored skill sync destinations to the sandbox `skills/` root and stop using frontmatter-controlled skill names as filesystem destination paths. Thanks @1seal. - Security/Web tools: treat browser/web content as untrusted by default (wrapped outputs for browser snapshot/tabs/console and structured external-content metadata for web tools), and strip `toolResult.details` from model-facing transcript/compaction inputs to reduce prompt-injection replay risk. - Security/Hooks: harden webhook and device token verification with shared constant-time secret comparison, and add per-client auth-failure throttling for hook endpoints (`429` + `Retry-After`). Thanks @akhmittra. - Gateway: raise WS payload/buffer limits so 5,000,000-byte image attachments work reliably. (#14486) Thanks @0xRaini. diff --git a/src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts b/src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts index 72cade4ae..507faa8f9 100644 --- a/src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts +++ b/src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts @@ -26,6 +26,15 @@ ${body ?? `# ${name}\n`} ); } +async function pathExists(filePath: string): Promise { + try { + await fs.access(filePath); + return true; + } catch { + return false; + } +} + describe("buildWorkspaceSkillsPrompt", () => { it("syncs merged skills into a target workspace", async () => { const sourceWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-")); @@ -74,6 +83,63 @@ describe("buildWorkspaceSkillsPrompt", () => { expect(prompt).not.toContain("Extra version"); expect(prompt).toContain(path.join(targetWorkspace, "skills", "demo-skill", "SKILL.md")); }); + it("keeps synced skills confined under target workspace when frontmatter name uses traversal", async () => { + const sourceWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-")); + const targetWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-")); + const escapeId = `${Date.now()}-${process.pid}-${Math.random().toString(16).slice(2)}`; + const traversalName = `../../../skill-sync-escape-${escapeId}`; + const escapedDest = path.resolve(targetWorkspace, "skills", traversalName); + + await writeSkill({ + dir: path.join(sourceWorkspace, "skills", "safe-traversal-skill"), + name: traversalName, + description: "Traversal skill", + }); + + expect(path.relative(path.join(targetWorkspace, "skills"), escapedDest).startsWith("..")).toBe( + true, + ); + expect(await pathExists(escapedDest)).toBe(false); + + await syncSkillsToWorkspace({ + sourceWorkspaceDir: sourceWorkspace, + targetWorkspaceDir: targetWorkspace, + bundledSkillsDir: path.join(sourceWorkspace, ".bundled"), + managedSkillsDir: path.join(sourceWorkspace, ".managed"), + }); + + expect( + await pathExists(path.join(targetWorkspace, "skills", "safe-traversal-skill", "SKILL.md")), + ).toBe(true); + expect(await pathExists(escapedDest)).toBe(false); + }); + it("keeps synced skills confined under target workspace when frontmatter name is absolute", async () => { + const sourceWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-")); + const targetWorkspace = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-")); + const escapeId = `${Date.now()}-${process.pid}-${Math.random().toString(16).slice(2)}`; + const absoluteDest = path.join(os.tmpdir(), `skill-sync-abs-escape-${escapeId}`); + + await fs.rm(absoluteDest, { recursive: true, force: true }); + await writeSkill({ + dir: path.join(sourceWorkspace, "skills", "safe-absolute-skill"), + name: absoluteDest, + description: "Absolute skill", + }); + + expect(await pathExists(absoluteDest)).toBe(false); + + await syncSkillsToWorkspace({ + sourceWorkspaceDir: sourceWorkspace, + targetWorkspaceDir: targetWorkspace, + bundledSkillsDir: path.join(sourceWorkspace, ".bundled"), + managedSkillsDir: path.join(sourceWorkspace, ".managed"), + }); + + expect( + await pathExists(path.join(targetWorkspace, "skills", "safe-absolute-skill", "SKILL.md")), + ).toBe(true); + expect(await pathExists(absoluteDest)).toBe(false); + }); it("filters skills based on env/config gates", async () => { const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-")); const skillDir = path.join(workspaceDir, "skills", "nano-banana-pro"); diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index fe6faf5ab..ee666eaca 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -16,6 +16,7 @@ import type { } from "./types.js"; import { createSubsystemLogger } from "../../logging/subsystem.js"; import { CONFIG_DIR, resolveUserPath } from "../../utils.js"; +import { resolveSandboxPath } from "../sandbox-paths.js"; import { resolveBundledSkillsDir } from "./bundled-dir.js"; import { shouldIncludeSkill } from "./config.js"; import { @@ -301,6 +302,45 @@ export function loadWorkspaceSkillEntries( return loadSkillEntries(workspaceDir, opts); } +function resolveUniqueSyncedSkillDirName(base: string, used: Set): string { + if (!used.has(base)) { + used.add(base); + return base; + } + for (let index = 2; index < 10_000; index += 1) { + const candidate = `${base}-${index}`; + if (!used.has(candidate)) { + used.add(candidate); + return candidate; + } + } + let fallbackIndex = 10_000; + let fallback = `${base}-${fallbackIndex}`; + while (used.has(fallback)) { + fallbackIndex += 1; + fallback = `${base}-${fallbackIndex}`; + } + used.add(fallback); + return fallback; +} + +function resolveSyncedSkillDestinationPath(params: { + targetSkillsDir: string; + entry: SkillEntry; + usedDirNames: Set; +}): string | null { + const sourceDirName = path.basename(params.entry.skill.baseDir).trim(); + if (!sourceDirName || sourceDirName === "." || sourceDirName === "..") { + return null; + } + const uniqueDirName = resolveUniqueSyncedSkillDirName(sourceDirName, params.usedDirNames); + return resolveSandboxPath({ + filePath: uniqueDirName, + cwd: params.targetSkillsDir, + root: params.targetSkillsDir, + }).resolved; +} + export async function syncSkillsToWorkspace(params: { sourceWorkspaceDir: string; targetWorkspaceDir: string; @@ -326,8 +366,28 @@ export async function syncSkillsToWorkspace(params: { await fsp.rm(targetSkillsDir, { recursive: true, force: true }); await fsp.mkdir(targetSkillsDir, { recursive: true }); + const usedDirNames = new Set(); for (const entry of entries) { - const dest = path.join(targetSkillsDir, entry.skill.name); + let dest: string | null = null; + try { + dest = resolveSyncedSkillDestinationPath({ + targetSkillsDir, + entry, + usedDirNames, + }); + } catch (error) { + const message = error instanceof Error ? error.message : JSON.stringify(error); + console.warn( + `[skills] Failed to resolve safe destination for ${entry.skill.name}: ${message}`, + ); + continue; + } + if (!dest) { + console.warn( + `[skills] Failed to resolve safe destination for ${entry.skill.name}: invalid source directory name`, + ); + continue; + } try { await fsp.cp(entry.skill.baseDir, dest, { recursive: true,