mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-26 04:15:48 -06:00
fix(security): stop recommending retired install-policy bypasses (#120011)
* fix(security): align install policy path contract * test(security): align install policy wording * style(security): format install policy test * fix(security): identify failing install policy path * test(security): track Windows ACL temp dirs
This commit is contained in:
committed by
GitHub
parent
b8b878ed33
commit
804ae7f121
@@ -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
|
||||
|
||||
@@ -168,16 +168,9 @@ skills, skill dependency installers, and plugin install/update sources.
|
||||
Optional allowlist of directories that may contain the policy executable.
|
||||
</ParamField>
|
||||
|
||||
<ParamField path="security.installPolicy.exec.allowInsecurePath" type="boolean" default="false">
|
||||
Bypasses command path ownership and permission checks. Use only when the
|
||||
path is protected by another mechanism.
|
||||
</ParamField>
|
||||
|
||||
<ParamField path="security.installPolicy.exec.allowSymlinkCommand" type="boolean" default="false">
|
||||
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.
|
||||
</ParamField>
|
||||
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`,
|
||||
|
||||
@@ -85,17 +85,7 @@ async function readJson<T>(filePath: string): Promise<T> {
|
||||
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,
|
||||
});
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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<string> {
|
||||
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<void> {
|
||||
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. */
|
||||
|
||||
@@ -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<typeof import("./audit-fs.js")>();
|
||||
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.`,
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user