diff --git a/CHANGELOG.md b/CHANGELOG.md index 2571cdc45cd..b66b2a08b05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Docs: https://docs.openclaw.ai - Security/Net: enforce strict dotted-decimal IPv4 literals in SSRF checks and fail closed on unsupported legacy forms (octal/hex/short/packed, for example `0177.0.0.1`, `127.1`, `2130706433`) before DNS lookup. - Security/Discord: enforce trusted-sender guild permission checks for moderation actions (`timeout`, `kick`, `ban`) and ignore untrusted `senderUserId` params to prevent privilege escalation in tool-driven flows. Thanks @aether-ai-agent for reporting. - Security/ACP+Exec: add `openclaw acp --token-file/--password-file` secret-file support (with inline secret flag warnings), redact ACP working-directory prefixes to `~` home-relative paths, constrain exec script preflight file inspection to the effective `workdir` boundary, and add security-audit warnings when `tools.exec.host="sandbox"` is configured while sandbox mode is off. +- Security/Plugins/Hooks: enforce runtime/package path containment with realpath checks so `openclaw.extensions`, `openclaw.hooks`, and hook handler modules cannot escape their trusted roots via traversal or symlinks. - Security/ACP: harden ACP bridge session management with duplicate-session refresh, idle-session reaping, oldest-idle soft-cap eviction, and burst rate limiting on session creation to reduce local DoS risk without disrupting normal IDE usage. - Security/Plugins/Hooks: add optional `--pin` for npm plugin/hook installs, persist resolved npm metadata (`name`, `version`, `spec`, integrity, shasum, timestamp), warn/confirm on integrity drift during updates, and extend `openclaw security audit` to flag unpinned specs, missing integrity metadata, and install-record version drift. - Security/Plugins: harden plugin discovery by blocking unsafe candidates (root escapes, world-writable paths, suspicious ownership), add startup warnings when `plugins.allow` is empty with discoverable non-bundled plugins, and warn on loaded plugins without install/load-path provenance. diff --git a/docs/automation/hooks.md b/docs/automation/hooks.md index 55c04e9990b..66b96cd1e9e 100644 --- a/docs/automation/hooks.md +++ b/docs/automation/hooks.md @@ -119,6 +119,8 @@ Example `package.json`: Each entry points to a hook directory containing `HOOK.md` and `handler.ts` (or `index.ts`). Hook packs can ship dependencies; they will be installed under `~/.openclaw/hooks/`. +Each `openclaw.hooks` entry must stay inside the package directory after symlink +resolution; entries that escape are rejected. Security note: `openclaw hooks install` installs dependencies with `npm install --ignore-scripts` (no lifecycle scripts). Keep hook pack dependency trees "pure JS/TS" and avoid packages that rely diff --git a/docs/tools/plugin.md b/docs/tools/plugin.md index 9433e9ea387..86a2b984316 100644 --- a/docs/tools/plugin.md +++ b/docs/tools/plugin.md @@ -151,6 +151,10 @@ becomes `name/`. If your plugin imports npm deps, install them in that directory so `node_modules` is available (`npm install` / `pnpm install`). +Security guardrail: every `openclaw.extensions` entry must stay inside the plugin +directory after symlink resolution. Entries that escape the package directory are +rejected. + Security note: `openclaw plugins install` installs plugin dependencies with `npm install --ignore-scripts` (no lifecycle scripts). Keep plugin dependency trees "pure JS/TS" and avoid packages that require `postinstall` builds. diff --git a/src/hooks/install.test.ts b/src/hooks/install.test.ts index b8ef3f761ef..e9c4d5bd8da 100644 --- a/src/hooks/install.test.ts +++ b/src/hooks/install.test.ts @@ -238,6 +238,74 @@ describe("installHooksFromPath", () => { expect(result.targetDir).toBe(path.join(stateDir, "hooks", "my-hook")); expect(fs.existsSync(path.join(result.targetDir, "HOOK.md"))).toBe(true); }); + + it("rejects hook pack entries that traverse outside package directory", async () => { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const pkgDir = path.join(workDir, "package"); + const outsideHookDir = path.join(workDir, "outside"); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.mkdirSync(outsideHookDir, { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ + name: "@openclaw/test-hooks", + version: "0.0.1", + openclaw: { hooks: ["../outside"] }, + }), + "utf-8", + ); + fs.writeFileSync(path.join(outsideHookDir, "HOOK.md"), "---\nname: outside\n---\n", "utf-8"); + fs.writeFileSync(path.join(outsideHookDir, "handler.ts"), "export default async () => {};\n"); + + const result = await installHooksFromPath({ + path: pkgDir, + hooksDir: path.join(stateDir, "hooks"), + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("openclaw.hooks entry escapes package directory"); + }); + + it("rejects hook pack entries that escape via symlink", async () => { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const pkgDir = path.join(workDir, "package"); + const outsideHookDir = path.join(workDir, "outside"); + const linkedDir = path.join(pkgDir, "linked"); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.mkdirSync(outsideHookDir, { recursive: true }); + fs.writeFileSync(path.join(outsideHookDir, "HOOK.md"), "---\nname: outside\n---\n", "utf-8"); + fs.writeFileSync(path.join(outsideHookDir, "handler.ts"), "export default async () => {};\n"); + try { + fs.symlinkSync(outsideHookDir, linkedDir, process.platform === "win32" ? "junction" : "dir"); + } catch { + return; + } + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ + name: "@openclaw/test-hooks", + version: "0.0.1", + openclaw: { hooks: ["./linked"] }, + }), + "utf-8", + ); + + const result = await installHooksFromPath({ + path: pkgDir, + hooksDir: path.join(stateDir, "hooks"), + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("openclaw.hooks entry resolves outside package directory"); + }); }); describe("installHooksFromNpmSpec", () => { diff --git a/src/hooks/install.ts b/src/hooks/install.ts index 6bd06146400..2eb3553363a 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -18,6 +18,7 @@ import { withTempDir, } from "../infra/install-source-utils.js"; import { validateRegistryNpmSpec } from "../infra/npm-registry-spec.js"; +import { isPathInside, isPathInsideWithRealpath } from "../security/scan-paths.js"; import { CONFIG_DIR, resolveUserPath } from "../utils.js"; import { parseFrontmatter } from "./frontmatter.js"; @@ -218,7 +219,23 @@ async function installHookPackageFromDir(params: { const resolvedHooks = [] as string[]; for (const entry of hookEntries) { const hookDir = path.resolve(params.packageDir, entry); + if (!isPathInside(params.packageDir, hookDir)) { + return { + ok: false, + error: `openclaw.hooks entry escapes package directory: ${entry}`, + }; + } await validateHookDir(hookDir); + if ( + !isPathInsideWithRealpath(params.packageDir, hookDir, { + requireRealpath: true, + }) + ) { + return { + ok: false, + error: `openclaw.hooks entry resolves outside package directory: ${entry}`, + }; + } const hookName = await resolveHookNameFromDir(hookDir); resolvedHooks.push(hookName); } diff --git a/src/hooks/loader.test.ts b/src/hooks/loader.test.ts index e9299b491f9..918e8098e4c 100644 --- a/src/hooks/loader.test.ts +++ b/src/hooks/loader.test.ts @@ -266,5 +266,73 @@ describe("loader", () => { // This test mainly verifies that loading and triggering doesn't crash expect(getRegisteredEventKeys()).toContain("command:new"); }); + + it("rejects directory hook handlers that escape hook dir via symlink", async () => { + const outsideHandlerPath = path.join(fixtureRoot, `outside-handler-${caseId}.js`); + await fs.writeFile(outsideHandlerPath, "export default async function() {}", "utf-8"); + + const hookDir = path.join(tmpDir, "hooks", "symlink-hook"); + await fs.mkdir(hookDir, { recursive: true }); + await fs.writeFile( + path.join(hookDir, "HOOK.md"), + [ + "---", + "name: symlink-hook", + "description: symlink test", + 'metadata: {"openclaw":{"events":["command:new"]}}', + "---", + "", + "# Symlink Hook", + ].join("\n"), + "utf-8", + ); + try { + await fs.symlink(outsideHandlerPath, path.join(hookDir, "handler.js")); + } catch { + return; + } + + const cfg: OpenClawConfig = { + hooks: { + internal: { + enabled: true, + }, + }, + }; + + const count = await loadInternalHooks(cfg, tmpDir); + expect(count).toBe(0); + expect(getRegisteredEventKeys()).not.toContain("command:new"); + }); + + it("rejects legacy handler modules that escape workspace via symlink", async () => { + const outsideHandlerPath = path.join(fixtureRoot, `outside-legacy-${caseId}.js`); + await fs.writeFile(outsideHandlerPath, "export default async function() {}", "utf-8"); + + const linkedHandlerPath = path.join(tmpDir, "legacy-handler.js"); + try { + await fs.symlink(outsideHandlerPath, linkedHandlerPath); + } catch { + return; + } + + const cfg: OpenClawConfig = { + hooks: { + internal: { + enabled: true, + handlers: [ + { + event: "command:new", + module: "legacy-handler.js", + }, + ], + }, + }, + }; + + const count = await loadInternalHooks(cfg, tmpDir); + expect(count).toBe(0); + expect(getRegisteredEventKeys()).not.toContain("command:new"); + }); }); }); diff --git a/src/hooks/loader.ts b/src/hooks/loader.ts index 391fdc12b69..342e74ac9af 100644 --- a/src/hooks/loader.ts +++ b/src/hooks/loader.ts @@ -9,6 +9,7 @@ import path from "node:path"; import { pathToFileURL } from "node:url"; import type { OpenClawConfig } from "../config/config.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; +import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { resolveHookConfig } from "./config.js"; import { shouldIncludeHook } from "./config.js"; import type { InternalHookHandler } from "./internal-hooks.js"; @@ -71,6 +72,16 @@ export async function loadInternalHooks( } try { + if ( + !isPathInsideWithRealpath(entry.hook.baseDir, entry.hook.handlerPath, { + requireRealpath: true, + }) + ) { + log.error( + `Hook '${entry.hook.name}' handler path resolves outside hook directory: ${entry.hook.handlerPath}`, + ); + continue; + } // Import handler module with cache-busting const url = pathToFileURL(entry.hook.handlerPath).href; const cacheBustedUrl = `${url}?t=${Date.now()}`; @@ -135,6 +146,16 @@ export async function loadInternalHooks( log.error(`Handler module path must stay within workspaceDir: ${rawModule}`); continue; } + if ( + !isPathInsideWithRealpath(baseDir, modulePath, { + requireRealpath: true, + }) + ) { + log.error( + `Handler module path resolves outside workspaceDir after symlink resolution: ${rawModule}`, + ); + continue; + } // Import the module with cache-busting to ensure fresh reload const url = pathToFileURL(modulePath).href; diff --git a/src/hooks/workspace.test.ts b/src/hooks/workspace.test.ts index 05a2dbbd8f6..cc35ffdd698 100644 --- a/src/hooks/workspace.test.ts +++ b/src/hooks/workspace.test.ts @@ -66,4 +66,40 @@ describe("hooks workspace", () => { const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); expect(entries.some((e) => e.hook.name === "nested")).toBe(true); }); + + it("ignores package.json hook paths that escape via symlink", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-link-")); + const hooksRoot = path.join(root, "hooks"); + fs.mkdirSync(hooksRoot, { recursive: true }); + + const pkgDir = path.join(hooksRoot, "pkg"); + const outsideDir = path.join(root, "outside"); + const linkedDir = path.join(pkgDir, "linked"); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, "HOOK.md"), "---\nname: outside\n---\n"); + fs.writeFileSync(path.join(outsideDir, "handler.js"), "export default async () => {};\n"); + try { + fs.symlinkSync(outsideDir, linkedDir, process.platform === "win32" ? "junction" : "dir"); + } catch { + return; + } + + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify( + { + name: "pkg", + [MANIFEST_KEY]: { + hooks: ["./linked"], + }, + }, + null, + 2, + ), + ); + + const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); + expect(entries.some((e) => e.hook.name === "outside")).toBe(false); + }); }); diff --git a/src/hooks/workspace.ts b/src/hooks/workspace.ts index 698ba3544af..fb2789a1abc 100644 --- a/src/hooks/workspace.ts +++ b/src/hooks/workspace.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { MANIFEST_KEY } from "../compat/legacy-names.js"; import type { OpenClawConfig } from "../config/config.js"; +import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { CONFIG_DIR, resolveUserPath } from "../utils.js"; import { resolveBundledHooksDir } from "./bundled-dir.js"; import { shouldIncludeHook } from "./config.js"; @@ -55,8 +56,11 @@ function resolvePackageHooks(manifest: HookPackageManifest): string[] { function resolveContainedDir(baseDir: string, targetDir: string): string | null { const base = path.resolve(baseDir); const resolved = path.resolve(baseDir, targetDir); - const relative = path.relative(base, resolved); - if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { + if ( + !isPathInsideWithRealpath(base, resolved, { + requireRealpath: true, + }) + ) { return null; } return resolved; diff --git a/src/plugins/discovery.test.ts b/src/plugins/discovery.test.ts index 11a5716461d..06cd7f36d0e 100644 --- a/src/plugins/discovery.test.ts +++ b/src/plugins/discovery.test.ts @@ -149,8 +149,7 @@ describe("discoverOpenClawPlugins", () => { const ids = candidates.map((c) => c.idHint); expect(ids).toContain("demo-plugin-dir"); }); - - it("blocks extension entries that escape plugin root", async () => { + it("blocks extension entries that escape package directory", async () => { const stateDir = makeTempDir(); const globalExt = path.join(stateDir, "extensions", "escape-pack"); const outside = path.join(stateDir, "outside.js"); @@ -172,10 +171,43 @@ describe("discoverOpenClawPlugins", () => { expect(result.candidates).toHaveLength(0); expect( - result.diagnostics.some((diag) => diag.message.includes("source escapes plugin root")), + result.diagnostics.some((diag) => diag.message.includes("escapes package directory")), ).toBe(true); }); + it("rejects package extension entries that escape via symlink", async () => { + const stateDir = makeTempDir(); + const globalExt = path.join(stateDir, "extensions", "pack"); + const outsideDir = path.join(stateDir, "outside"); + const linkedDir = path.join(globalExt, "linked"); + fs.mkdirSync(globalExt, { recursive: true }); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, "escape.ts"), "export default {}", "utf-8"); + try { + fs.symlinkSync(outsideDir, linkedDir, process.platform === "win32" ? "junction" : "dir"); + } catch { + return; + } + + fs.writeFileSync( + path.join(globalExt, "package.json"), + JSON.stringify({ + name: "@openclaw/pack", + openclaw: { extensions: ["./linked/escape.ts"] }, + }), + "utf-8", + ); + + const { candidates, diagnostics } = await withStateDir(stateDir, async () => { + return discoverOpenClawPlugins({}); + }); + + expect(candidates.some((candidate) => candidate.idHint === "pack")).toBe(false); + expect(diagnostics.some((entry) => entry.message.includes("escapes package directory"))).toBe( + true, + ); + }); + it.runIf(process.platform !== "win32")("blocks world-writable plugin paths", async () => { const stateDir = makeTempDir(); const globalExt = path.join(stateDir, "extensions"); diff --git a/src/plugins/discovery.ts b/src/plugins/discovery.ts index 747508142ab..932602107e6 100644 --- a/src/plugins/discovery.ts +++ b/src/plugins/discovery.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import path from "node:path"; +import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { resolveConfigDir, resolveUserPath } from "../utils.js"; import { resolveBundledPluginsDir } from "./bundled-dir.js"; import { @@ -294,6 +295,28 @@ function addCandidate(params: { }); } +function resolvePackageEntrySource(params: { + packageDir: string; + entryPath: string; + sourceLabel: string; + diagnostics: PluginDiagnostic[]; +}): string | null { + const source = path.resolve(params.packageDir, params.entryPath); + if ( + !isPathInsideWithRealpath(params.packageDir, source, { + requireRealpath: true, + }) + ) { + params.diagnostics.push({ + level: "error", + message: `extension entry escapes package directory: ${params.entryPath}`, + source: params.sourceLabel, + }); + return null; + } + return source; +} + function discoverInDirectory(params: { dir: string; origin: PluginOrigin; @@ -345,7 +368,15 @@ function discoverInDirectory(params: { if (extensions.length > 0) { for (const extPath of extensions) { - const resolved = path.resolve(fullPath, extPath); + const resolved = resolvePackageEntrySource({ + packageDir: fullPath, + entryPath: extPath, + sourceLabel: fullPath, + diagnostics: params.diagnostics, + }); + if (!resolved) { + continue; + } addCandidate({ candidates: params.candidates, diagnostics: params.diagnostics, @@ -438,7 +469,15 @@ function discoverFromPath(params: { if (extensions.length > 0) { for (const extPath of extensions) { - const source = path.resolve(resolved, extPath); + const source = resolvePackageEntrySource({ + packageDir: resolved, + entryPath: extPath, + sourceLabel: resolved, + diagnostics: params.diagnostics, + }); + if (!source) { + continue; + } addCandidate({ candidates: params.candidates, diagnostics: params.diagnostics, diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 0af06b0df9e..0d2567a93a3 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -489,7 +489,6 @@ describe("loadOpenClawPlugins", () => { expect(loaded?.origin).toBe("config"); expect(overridden?.origin).toBe("bundled"); }); - it("warns when plugins.allow is empty and non-bundled plugins are discoverable", () => { process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; const plugin = writePlugin({ @@ -561,4 +560,48 @@ describe("loadOpenClawPlugins", () => { } } }); + + it("rejects plugin entry files that escape plugin root via symlink", () => { + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + const pluginDir = makeTempDir(); + const outsideDir = makeTempDir(); + const outsideEntry = path.join(outsideDir, "outside.js"); + const linkedEntry = path.join(pluginDir, "entry.js"); + fs.writeFileSync( + outsideEntry, + 'export default { id: "symlinked", register() { throw new Error("should not run"); } };', + "utf-8", + ); + fs.writeFileSync( + path.join(pluginDir, "openclaw.plugin.json"), + JSON.stringify( + { + id: "symlinked", + configSchema: EMPTY_PLUGIN_SCHEMA, + }, + null, + 2, + ), + "utf-8", + ); + try { + fs.symlinkSync(outsideEntry, linkedEntry); + } catch { + return; + } + + const registry = loadOpenClawPlugins({ + cache: false, + config: { + plugins: { + load: { paths: [linkedEntry] }, + allow: ["symlinked"], + }, + }, + }); + + const record = registry.plugins.find((entry) => entry.id === "symlinked"); + expect(record?.status).not.toBe("loaded"); + expect(registry.diagnostics.some((entry) => entry.message.includes("escapes"))).toBe(true); + }); }); diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index c88483e8682..9298a9cd70f 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -5,6 +5,7 @@ import { createJiti } from "jiti"; import type { OpenClawConfig } from "../config/config.js"; import type { GatewayRequestHandler } from "../gateway/server-methods/types.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; +import { isPathInsideWithRealpath } from "../security/scan-paths.js"; import { resolveUserPath } from "../utils.js"; import { clearPluginCommands } from "./commands.js"; import { @@ -485,6 +486,24 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi continue; } + if ( + !isPathInsideWithRealpath(candidate.rootDir, candidate.source, { + requireRealpath: true, + }) + ) { + record.status = "error"; + record.error = "plugin entry path escapes plugin root"; + registry.plugins.push(record); + seenIds.set(pluginId, candidate.origin); + registry.diagnostics.push({ + level: "error", + pluginId: record.id, + source: record.source, + message: record.error, + }); + continue; + } + let mod: OpenClawPluginModule | null = null; try { mod = getJiti()(candidate.source) as OpenClawPluginModule; diff --git a/src/security/scan-paths.ts b/src/security/scan-paths.ts index 246df3fefbc..e41e7ade261 100644 --- a/src/security/scan-paths.ts +++ b/src/security/scan-paths.ts @@ -1,3 +1,4 @@ +import fs from "node:fs"; import path from "node:path"; export function isPathInside(basePath: string, candidatePath: string): boolean { @@ -7,6 +8,30 @@ export function isPathInside(basePath: string, candidatePath: string): boolean { return rel === "" || (!rel.startsWith(`..${path.sep}`) && rel !== ".." && !path.isAbsolute(rel)); } +function safeRealpathSync(filePath: string): string | null { + try { + return fs.realpathSync(filePath); + } catch { + return null; + } +} + +export function isPathInsideWithRealpath( + basePath: string, + candidatePath: string, + opts?: { requireRealpath?: boolean }, +): boolean { + if (!isPathInside(basePath, candidatePath)) { + return false; + } + const baseReal = safeRealpathSync(basePath); + const candidateReal = safeRealpathSync(candidatePath); + if (!baseReal || !candidateReal) { + return opts?.requireRealpath !== true; + } + return isPathInside(baseReal, candidateReal); +} + export function extensionUsesSkippedScannerPath(entry: string): boolean { const segments = entry.split(/[\\/]+/).filter(Boolean); return segments.some(