diff --git a/config/max-lines-baseline.txt b/config/max-lines-baseline.txt index 8ce849bace60..d3fbf809bb37 100644 --- a/config/max-lines-baseline.txt +++ b/config/max-lines-baseline.txt @@ -900,7 +900,6 @@ src/secrets/runtime-web-tools.ts src/security/audit-extra.async.ts src/security/audit-extra.sync.ts src/security/audit.ts -src/security/install-policy.ts src/sessions/session-state-events.ts src/shared/json-schema-defaults.ts src/shared/text/assistant-visible-text.ts diff --git a/docs/tools/skills-config.md b/docs/tools/skills-config.md index 92dfd6dacd15..690aa4ddace4 100644 --- a/docs/tools/skills-config.md +++ b/docs/tools/skills-config.md @@ -168,16 +168,9 @@ skills, skill dependency installers, and plugin install/update sources. Optional allowlist of directories that may contain the policy executable. - - Bypasses command path ownership and permission checks. Use only when the - path is protected by another mechanism. - - - - Allows the configured command path to be a symlink. The resolved target - must still satisfy the other path checks. Interpreter script arguments must - be direct regular files, not symlinks. - +The policy command and interpreter script arguments must be direct regular +files with trusted ownership, restricted permissions, and verifiable parent +directories. Symlinks and insecure paths are rejected. The policy receives one JSON object on stdin with `protocolVersion: 1`, `openclawVersion`, `targetType`, `targetName`, `sourcePath`, `sourcePathKind`, diff --git a/src/plugins/install.npm-spec.e2e.test.ts b/src/plugins/install.npm-spec.e2e.test.ts index 991e00fb2bde..7eb953933aa7 100644 --- a/src/plugins/install.npm-spec.e2e.test.ts +++ b/src/plugins/install.npm-spec.e2e.test.ts @@ -85,17 +85,7 @@ async function readJson(filePath: string): Promise { return JSON.parse(await fs.readFile(filePath, "utf8")) as T; } -function configWithInstalledPackageTreeBlockPolicy(): OpenClawConfig { - return { - security: { - installPolicy: { - enabled: true, - exec: { - source: "exec", - command: process.execPath, - args: [ - "-e", - ` +const installedPackageTreePolicySource = ` let input = ""; process.stdin.setEncoding("utf8"); process.stdin.on("data", (chunk) => { input += chunk; }); @@ -111,8 +101,30 @@ process.stdin.on("end", () => { } process.stdout.write(JSON.stringify({ protocolVersion: 1, decision: "allow" })); }); -`, - ], +`; + +async function createInstalledPackageTreePolicyExec(rootDir: string) { + if (process.platform === "win32") { + return { command: process.execPath, args: ["-e", installedPackageTreePolicySource] }; + } + const command = path.join(rootDir, "install-policy.cjs"); + await fs.writeFile(command, `#!${process.execPath}\n${installedPackageTreePolicySource}`, "utf8"); + await fs.chmod(command, 0o700); + return { command, args: [] }; +} + +function configWithInstalledPackageTreeBlockPolicy(exec: { + command: string; + args: string[]; +}): OpenClawConfig { + return { + security: { + installPolicy: { + enabled: true, + exec: { + source: "exec", + command: exec.command, + args: exec.args, timeoutMs: 5000, maxOutputBytes: 16 * 1024, }, @@ -721,6 +733,7 @@ describe("installPluginFromNpmSpec e2e", () => { it("rolls back managed peer dependencies added before a failed installed package policy scan", async () => { const { rootDir, npmRoot } = await makeInstallFixture("npm-plugin-peer-rollback-e2e"); + const policyExec = await createInstalledPackageTreePolicyExec(rootDir); const blockedPlugin = uniquePackageName("blocked-plugin"); const runtimePeer = uniquePackageName("runtime-peer"); await useStaticRegistry([ @@ -735,7 +748,7 @@ describe("installPluginFromNpmSpec e2e", () => { ]); const result = await installNpmPlugin({ - config: configWithInstalledPackageTreeBlockPolicy(), + config: configWithInstalledPackageTreeBlockPolicy(policyExec), spec: `${blockedPlugin}@1.0.0`, npmRoot, }); @@ -799,6 +812,7 @@ describe("installPluginFromNpmSpec e2e", () => { it("does not take ownership of an existing root dependency observed as a peer", async () => { const { rootDir, npmRoot } = await makeInstallFixture("npm-plugin-peer-existing-root-e2e"); + const policyExec = await createInstalledPackageTreePolicyExec(rootDir); const existingRootDependency = uniquePackageName("existing-root"); const blockedPlugin = uniquePackageName("blocked-plugin"); const runtimePeer = uniquePackageName("runtime-peer"); @@ -823,7 +837,7 @@ describe("installPluginFromNpmSpec e2e", () => { }); const result = await installNpmPlugin({ - config: configWithInstalledPackageTreeBlockPolicy(), + config: configWithInstalledPackageTreeBlockPolicy(policyExec), spec: `${blockedPlugin}@1.0.0`, npmRoot, }); diff --git a/src/security/install-policy.test.ts b/src/security/install-policy.test.ts index e06056d68e39..be5888491fe6 100644 --- a/src/security/install-policy.test.ts +++ b/src/security/install-policy.test.ts @@ -642,32 +642,29 @@ describe("runInstallPolicy", () => { ); }); - it.runIf(process.platform !== "win32")( - "rejects symlinked interpreter script args even when command symlinks are allowed", - async () => { - const dir = await makeTempDir(); - const realScriptPath = await writePolicyScript(dir); - const symlinkScriptPath = path.join(dir, "policy-link.cjs"); - await fs.symlink(realScriptPath, symlinkScriptPath); + it.runIf(process.platform !== "win32")("rejects symlinked interpreter script args", async () => { + const dir = await makeTempDir(); + const realScriptPath = await writePolicyScript(dir); + const symlinkScriptPath = path.join(dir, "policy-link.cjs"); + await fs.symlink(realScriptPath, symlinkScriptPath); - const validation = await validateInstallPolicyStatic({ - security: { - installPolicy: { - enabled: true, - exec: { - source: "exec", - command: process.execPath, - args: [symlinkScriptPath], - }, + const validation = await validateInstallPolicyStatic({ + security: { + installPolicy: { + enabled: true, + exec: { + source: "exec", + command: process.execPath, + args: [symlinkScriptPath], }, }, - }); + }, + }); - expect(validation.issues.map((issue) => issue.message)).toContain( - `security.installPolicy.exec.args[0] must not be a symlink: ${symlinkScriptPath}`, - ); - }, - ); + expect(validation.issues.map((issue) => issue.message)).toContain( + `security.installPolicy.exec.args[0] must not be a symlink: ${symlinkScriptPath}`, + ); + }); it.runIf(process.platform !== "win32")( "rejects env policy commands before interpreter resolution can bypass validation", diff --git a/src/security/install-policy.ts b/src/security/install-policy.ts index 1e3319607d13..339398465722 100644 --- a/src/security/install-policy.ts +++ b/src/security/install-policy.ts @@ -267,7 +267,7 @@ async function assertSecureCommandAncestorDirs(params: { } if (process.platform === "win32" && perms.source === "unknown") { throw new Error( - `${params.label} parent directory ACL verification unavailable on Windows for ${dir}. Set allowInsecurePath=true for this policy to bypass this check when the path is trusted.`, + `${params.label} parent directory ACL verification unavailable on Windows for ${dir}. Move ${params.label} to a direct path whose ACLs can be verified.`, ); } } @@ -277,31 +277,15 @@ async function assertSecureCommandPath(params: { targetPath: string; label: string; trustedDirs?: string[]; - allowInsecurePath?: boolean; - allowSymlinkPath?: boolean; }): Promise { if (!isAbsolutePathname(params.targetPath)) { throw new Error(`${params.label} must be an absolute path.`); } - let effectivePath = params.targetPath; - let stat = await readFileStatOrThrow(effectivePath, params.label); + const effectivePath = params.targetPath; + const stat = await readFileStatOrThrow(effectivePath, params.label); if (stat.isSymlink) { - if (!params.allowSymlinkPath) { - throw new Error(`${params.label} must not be a symlink: ${effectivePath}`); - } - try { - effectivePath = await fs.realpath(effectivePath); - } catch { - throw new Error(`${params.label} symlink target is not readable: ${params.targetPath}`); - } - if (!isAbsolutePathname(effectivePath)) { - throw new Error(`${params.label} resolved symlink target must be an absolute path.`); - } - stat = await readFileStatOrThrow(effectivePath, params.label); - if (stat.isSymlink) { - throw new Error(`${params.label} symlink target must not be a symlink: ${effectivePath}`); - } + throw new Error(`${params.label} must not be a symlink: ${effectivePath}`); } if (params.trustedDirs && params.trustedDirs.length > 0) { @@ -311,10 +295,6 @@ async function assertSecureCommandPath(params: { throw new Error(`${params.label} is outside trustedDirs: ${effectivePath}`); } } - if (params.allowInsecurePath) { - return effectivePath; - } - const perms = await inspectPathPermissions(effectivePath); if (!perms.ok) { throw new Error(`${params.label} permissions could not be verified: ${effectivePath}`); @@ -326,7 +306,7 @@ async function assertSecureCommandPath(params: { if (process.platform === "win32" && perms.source === "unknown") { throw new Error( - `${params.label} ACL verification unavailable on Windows for ${effectivePath}. Set allowInsecurePath=true for this policy to bypass this check when the path is trusted.`, + `${params.label} ACL verification unavailable on Windows for ${effectivePath}. Move ${params.label} to a direct path whose ACLs can be verified.`, ); } @@ -345,8 +325,6 @@ async function assertSecurePolicyScriptArg(params: { command: string; args: string[]; trustedDirs?: string[]; - allowInsecurePath?: boolean; - allowSymlinkPath?: boolean; }): Promise { const scriptArg = resolvePolicyScriptArg({ command: params.command, args: params.args }); if (!scriptArg) { @@ -360,8 +338,6 @@ async function assertSecurePolicyScriptArg(params: { targetPath: script.path, label: `security.installPolicy.exec.args[${script.index}]`, trustedDirs: params.trustedDirs, - allowInsecurePath: params.allowInsecurePath, - allowSymlinkPath: false, }); } } @@ -476,7 +452,6 @@ export async function validateInstallPolicyStatic( targetPath: policy.exec.command, label: "security.installPolicy.exec.command", trustedDirs: policy.exec.trustedDirs, - allowSymlinkPath: false, }); } catch (err) { issues.push({ @@ -489,7 +464,6 @@ export async function validateInstallPolicyStatic( command: policy.exec.command, args: policy.exec.args ?? [], trustedDirs: policy.exec.trustedDirs, - allowSymlinkPath: false, }); } catch (err) { issues.push({ @@ -625,7 +599,6 @@ export async function runInstallPolicy(params: { targetPath: commandPath, label: "security.installPolicy.exec.command", trustedDirs: policy.exec.trustedDirs, - allowSymlinkPath: false, }); } catch (err) { return failClosed(formatErrorMessage(err)); @@ -635,7 +608,6 @@ export async function runInstallPolicy(params: { command: secureCommandPath, args: policy.exec.args ?? [], trustedDirs: policy.exec.trustedDirs, - allowSymlinkPath: false, }); } catch (err) { return failClosed(formatErrorMessage(err)); @@ -746,4 +718,3 @@ export async function probeInstallPolicy(params: { }, }); } -/* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */ diff --git a/src/security/install-policy.windows.test.ts b/src/security/install-policy.windows.test.ts new file mode 100644 index 000000000000..d0c3e1537602 --- /dev/null +++ b/src/security/install-policy.windows.test.ts @@ -0,0 +1,77 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; +import type { PermissionCheck } from "./audit-fs.js"; +import { validateInstallPolicyStatic } from "./install-policy.js"; + +const auditMocks = vi.hoisted(() => ({ + inspectPathPermissions: vi.fn(), +})); + +vi.mock("./audit-fs.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + inspectPathPermissions: auditMocks.inspectPathPermissions, + }; +}); + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); + +function permissions(source: PermissionCheck["source"]): PermissionCheck { + return { + ok: true, + isSymlink: false, + isDir: false, + mode: 0o700, + bits: 0, + source, + worldWritable: false, + groupWritable: false, + worldReadable: false, + groupReadable: false, + }; +} + +beforeEach(() => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + auditMocks.inspectPathPermissions.mockResolvedValue(permissions("windows-acl")); +}); + +afterEach(() => { + vi.restoreAllMocks(); + auditMocks.inspectPathPermissions.mockReset(); +}); + +describe("install policy Windows ACL diagnostics", () => { + it.each([ + { kind: "file", unavailablePath: (scriptPath: string) => scriptPath }, + { kind: "parent directory", unavailablePath: (scriptPath: string) => path.dirname(scriptPath) }, + ])("identifies the interpreter script when its $kind ACL is unavailable", async (fixture) => { + const dir = tempDirs.make("openclaw-install-policy-windows-"); + const scriptPath = path.join(dir, "policy.cjs"); + await fs.writeFile(scriptPath, "export {};\n", "utf8"); + const unavailablePath = fixture.unavailablePath(scriptPath); + auditMocks.inspectPathPermissions.mockImplementation(async (targetPath: string) => + permissions(targetPath === unavailablePath ? "unknown" : "windows-acl"), + ); + + const validation = await validateInstallPolicyStatic({ + security: { + installPolicy: { + enabled: true, + exec: { + source: "exec", + command: process.execPath, + args: [scriptPath], + }, + }, + }, + }); + + expect(validation.issues.map((issue) => issue.message)).toContain( + `security.installPolicy.exec.args[0]${fixture.kind === "parent directory" ? " parent directory" : ""} ACL verification unavailable on Windows for ${unavailablePath}. Move security.installPolicy.exec.args[0] to a direct path whose ACLs can be verified.`, + ); + }); +});