fix(security): block shell env allowlist bypass in system.run

This commit is contained in:
Peter Steinberger
2026-02-22 12:46:55 +01:00
parent d5bb9f026e
commit e80c803fa8
12 changed files with 242 additions and 20 deletions

View File

@@ -11,6 +11,8 @@
"BASH_ENV",
"ENV",
"SHELL",
"SHELLOPTS",
"PS4",
"GCONV_PATH",
"IFS",
"SSLKEYLOGFILE"

View File

@@ -1,9 +1,14 @@
import { spawn } from "node:child_process";
import fs from "node:fs";
import os from "node:os";
import path from "node:path";
import { describe, expect, it } from "vitest";
import {
isDangerousHostEnvOverrideVarName,
isDangerousHostEnvVarName,
normalizeEnvVarKey,
sanitizeHostExecEnv,
sanitizeSystemRunEnvOverrides,
} from "./host-env-security.js";
describe("isDangerousHostEnvVarName", () => {
@@ -11,6 +16,8 @@ describe("isDangerousHostEnvVarName", () => {
expect(isDangerousHostEnvVarName("BASH_ENV")).toBe(true);
expect(isDangerousHostEnvVarName("bash_env")).toBe(true);
expect(isDangerousHostEnvVarName("SHELL")).toBe(true);
expect(isDangerousHostEnvVarName("SHELLOPTS")).toBe(true);
expect(isDangerousHostEnvVarName("ps4")).toBe(true);
expect(isDangerousHostEnvVarName("DYLD_INSERT_LIBRARIES")).toBe(true);
expect(isDangerousHostEnvVarName("ld_preload")).toBe(true);
expect(isDangerousHostEnvVarName("BASH_FUNC_echo%%")).toBe(true);
@@ -48,17 +55,37 @@ describe("sanitizeHostExecEnv", () => {
HOME: "/tmp/evil-home",
ZDOTDIR: "/tmp/evil-zdotdir",
BASH_ENV: "/tmp/pwn.sh",
SHELLOPTS: "xtrace",
PS4: "$(touch /tmp/pwned)",
SAFE: "ok",
},
});
expect(env.PATH).toBe("/usr/bin:/bin");
expect(env.BASH_ENV).toBeUndefined();
expect(env.SHELLOPTS).toBeUndefined();
expect(env.PS4).toBeUndefined();
expect(env.SAFE).toBe("ok");
expect(env.HOME).toBe("/tmp/trusted-home");
expect(env.ZDOTDIR).toBe("/tmp/trusted-zdotdir");
});
it("drops dangerous inherited shell trace keys", () => {
const env = sanitizeHostExecEnv({
baseEnv: {
PATH: "/usr/bin:/bin",
SHELLOPTS: "xtrace",
PS4: "$(touch /tmp/pwned)",
OK: "1",
},
});
expect(env.PATH).toBe("/usr/bin:/bin");
expect(env.OK).toBe("1");
expect(env.SHELLOPTS).toBeUndefined();
expect(env.PS4).toBeUndefined();
});
it("drops non-portable env key names", () => {
const env = sanitizeHostExecEnv({
baseEnv: {
@@ -94,3 +121,72 @@ describe("normalizeEnvVarKey", () => {
expect(normalizeEnvVarKey(" ")).toBeNull();
});
});
describe("sanitizeSystemRunEnvOverrides", () => {
it("keeps overrides for non-shell commands", () => {
const overrides = sanitizeSystemRunEnvOverrides({
shellWrapper: false,
overrides: {
OPENCLAW_TEST: "1",
TOKEN: "abc",
},
});
expect(overrides).toEqual({
OPENCLAW_TEST: "1",
TOKEN: "abc",
});
});
it("drops non-allowlisted overrides for shell wrappers", () => {
const overrides = sanitizeSystemRunEnvOverrides({
shellWrapper: true,
overrides: {
OPENCLAW_TEST: "1",
TOKEN: "abc",
LANG: "C",
LC_ALL: "C",
},
});
expect(overrides).toEqual({
LANG: "C",
LC_ALL: "C",
});
});
});
describe("shell wrapper exploit regression", () => {
it("blocks SHELLOPTS/PS4 chain after sanitization", async () => {
const bashPath = "/bin/bash";
if (process.platform === "win32" || !fs.existsSync(bashPath)) {
return;
}
const marker = path.join(os.tmpdir(), `openclaw-ps4-marker-${process.pid}-${Date.now()}`);
try {
fs.unlinkSync(marker);
} catch {
// no-op
}
const filteredOverrides = sanitizeSystemRunEnvOverrides({
shellWrapper: true,
overrides: {
SHELLOPTS: "xtrace",
PS4: `$(touch ${marker})`,
},
});
const env = sanitizeHostExecEnv({
overrides: filteredOverrides,
baseEnv: {
PATH: process.env.PATH ?? "/usr/bin:/bin",
},
});
await new Promise<void>((resolve, reject) => {
const child = spawn(bashPath, ["-lc", "echo SAFE"], { env, stdio: "ignore" });
child.once("error", reject);
child.once("close", () => resolve());
});
expect(fs.existsSync(marker)).toBe(false);
});
});

View File

@@ -19,10 +19,23 @@ export const HOST_DANGEROUS_ENV_PREFIXES: readonly string[] = Object.freeze(
export const HOST_DANGEROUS_OVERRIDE_ENV_KEY_VALUES: readonly string[] = Object.freeze(
(HOST_ENV_SECURITY_POLICY.blockedOverrideKeys ?? []).map((key) => key.toUpperCase()),
);
export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES: readonly string[] = Object.freeze([
"TERM",
"LANG",
"LC_ALL",
"LC_CTYPE",
"LC_MESSAGES",
"COLORTERM",
"NO_COLOR",
"FORCE_COLOR",
]);
export const HOST_DANGEROUS_ENV_KEYS = new Set<string>(HOST_DANGEROUS_ENV_KEY_VALUES);
export const HOST_DANGEROUS_OVERRIDE_ENV_KEYS = new Set<string>(
HOST_DANGEROUS_OVERRIDE_ENV_KEY_VALUES,
);
export const HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS = new Set<string>(
HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEY_VALUES,
);
export function normalizeEnvVarKey(
rawKey: string,
@@ -105,3 +118,31 @@ export function sanitizeHostExecEnv(params?: {
return merged;
}
export function sanitizeSystemRunEnvOverrides(params?: {
overrides?: Record<string, string> | null;
shellWrapper?: boolean;
}): Record<string, string> | undefined {
const overrides = params?.overrides ?? undefined;
if (!overrides) {
return undefined;
}
if (!params?.shellWrapper) {
return overrides;
}
const filtered: Record<string, string> = {};
for (const [rawKey, value] of Object.entries(overrides)) {
if (typeof value !== "string") {
continue;
}
const key = normalizeEnvVarKey(rawKey, { portable: true });
if (!key) {
continue;
}
if (!HOST_SHELL_WRAPPER_ALLOWED_OVERRIDE_ENV_KEYS.has(key.toUpperCase())) {
continue;
}
filtered[key] = value;
}
return Object.keys(filtered).length > 0 ? filtered : undefined;
}

View File

@@ -19,6 +19,7 @@ import {
} from "../infra/exec-approvals.js";
import type { ExecHostRequest, ExecHostResponse, ExecHostRunResult } from "../infra/exec-host.js";
import { getTrustedSafeBinDirs } from "../infra/exec-safe-bin-trust.js";
import { sanitizeSystemRunEnvOverrides } from "../infra/host-env-security.js";
import { resolveSystemRunCommand } from "../infra/system-run-command.js";
import type {
ExecEventPayload,
@@ -109,7 +110,11 @@ export async function handleSystemRunInvoke(opts: {
const autoAllowSkills = approvals.agent.autoAllowSkills;
const sessionKey = opts.params.sessionKey?.trim() || "node";
const runId = opts.params.runId?.trim() || crypto.randomUUID();
const env = opts.sanitizeEnv(opts.params.env ?? undefined);
const envOverrides = sanitizeSystemRunEnvOverrides({
overrides: opts.params.env ?? undefined,
shellWrapper: shellCommand !== null,
});
const env = opts.sanitizeEnv(envOverrides);
const safeBins = resolveSafeBins(agentExec?.safeBins ?? cfg.tools?.exec?.safeBins);
const trustedSafeBinDirs = getTrustedSafeBinDirs();
const bins = autoAllowSkills ? await opts.skillBins.current() : new Set<string>();
@@ -171,7 +176,7 @@ export async function handleSystemRunInvoke(opts: {
command: argv,
rawCommand: rawCommand || shellCommand || null,
cwd: opts.params.cwd ?? null,
env: opts.params.env ?? null,
env: envOverrides ?? null,
timeoutMs: opts.params.timeoutMs ?? null,
needsScreenRecording: opts.params.needsScreenRecording ?? null,
agentId: agentId ?? null,

View File

@@ -12,18 +12,25 @@ describe("node-host sanitizeEnv", () => {
});
it("blocks dangerous env keys/prefixes", () => {
withEnv({ PYTHONPATH: undefined, LD_PRELOAD: undefined, BASH_ENV: undefined }, () => {
const env = sanitizeEnv({
PYTHONPATH: "/tmp/pwn",
LD_PRELOAD: "/tmp/pwn.so",
BASH_ENV: "/tmp/pwn.sh",
FOO: "bar",
});
expect(env.FOO).toBe("bar");
expect(env.PYTHONPATH).toBeUndefined();
expect(env.LD_PRELOAD).toBeUndefined();
expect(env.BASH_ENV).toBeUndefined();
});
withEnv(
{ PYTHONPATH: undefined, LD_PRELOAD: undefined, BASH_ENV: undefined, SHELLOPTS: undefined },
() => {
const env = sanitizeEnv({
PYTHONPATH: "/tmp/pwn",
LD_PRELOAD: "/tmp/pwn.so",
BASH_ENV: "/tmp/pwn.sh",
SHELLOPTS: "xtrace",
PS4: "$(touch /tmp/pwned)",
FOO: "bar",
});
expect(env.FOO).toBe("bar");
expect(env.PYTHONPATH).toBeUndefined();
expect(env.LD_PRELOAD).toBeUndefined();
expect(env.BASH_ENV).toBeUndefined();
expect(env.SHELLOPTS).toBeUndefined();
expect(env.PS4).toBeUndefined();
},
);
});
it("blocks dangerous override-only env keys", () => {