From 25e89cc86338ef475d26be043aa541dfdb95e52a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 20:01:00 +0100 Subject: [PATCH] fix(security): harden shell env fallback --- CHANGELOG.md | 1 + .../Sources/OpenClaw/HostEnvSanitizer.swift | 1 + src/agents/skills.e2e.test.ts | 11 +++- src/config/config.env-vars.test.ts | 33 ++++++---- src/infra/host-env-security-policy.json | 1 + src/infra/host-env-security.test.ts | 1 + src/infra/shell-env.test.ts | 32 ++++++++++ src/infra/shell-env.ts | 62 ++++++++++++++++++- 8 files changed, 129 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 395d6d180..56855a620 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Security/Shell env: validate login-shell executable paths for shell-env fallback (`/etc/shells` + trusted prefixes) and block `SHELL` in dangerous env override policy paths so untrusted shell-path injection falls back safely to `/bin/sh`. Thanks @athuljayaram for reporting. - Chat/Usage/TUI: strip synthetic inbound metadata blocks (including `Conversation info` and trailing `Untrusted context` channel metadata wrappers) from displayed conversation history so internal prompt context no longer leaks into user-visible logs. - Security/Exec: in non-default setups that manually add `sort` to `tools.exec.safeBins`, block `sort --compress-program` so allowlist-mode safe-bin checks cannot bypass approval. Thanks @tdjackey for reporting. - Security/Discord: add `openclaw security audit` warnings for name/tag-based Discord allowlist entries (DM allowlists, guild/channel `users`, and pairing-store entries), highlighting slug-collision risk while keeping name-based matching supported, and canonicalize resolved Discord allowlist names to IDs at runtime without rewriting config files. Thanks @tdjackey for reporting. diff --git a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift index 0171de793..b387c36d3 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSanitizer.swift @@ -14,6 +14,7 @@ enum HostEnvSanitizer { "RUBYOPT", "BASH_ENV", "ENV", + "SHELL", "GCONV_PATH", "IFS", "SSLKEYLOGFILE", diff --git a/src/agents/skills.e2e.test.ts b/src/agents/skills.e2e.test.ts index d722e068f..4d5fb0c80 100644 --- a/src/agents/skills.e2e.test.ts +++ b/src/agents/skills.e2e.test.ts @@ -360,7 +360,7 @@ describe("applySkillEnvOverrides", () => { dir: skillDir, name: "dangerous-env-skill", description: "Needs env", - metadata: '{"openclaw":{"requires":{"env":["BASH_ENV"]}}}', + metadata: '{"openclaw":{"requires":{"env":["BASH_ENV","SHELL"]}}}', }); const entries = loadWorkspaceSkillEntries(workspaceDir, { @@ -368,7 +368,9 @@ describe("applySkillEnvOverrides", () => { }); const originalBashEnv = process.env.BASH_ENV; + const originalShell = process.env.SHELL; delete process.env.BASH_ENV; + delete process.env.SHELL; const restore = applySkillEnvOverrides({ skills: entries, @@ -378,6 +380,7 @@ describe("applySkillEnvOverrides", () => { "dangerous-env-skill": { env: { BASH_ENV: "/tmp/pwn.sh", + SHELL: "/tmp/evil-shell", }, }, }, @@ -387,6 +390,7 @@ describe("applySkillEnvOverrides", () => { try { expect(process.env.BASH_ENV).toBeUndefined(); + expect(process.env.SHELL).toBeUndefined(); } finally { restore(); if (originalBashEnv === undefined) { @@ -394,6 +398,11 @@ describe("applySkillEnvOverrides", () => { } else { expect(process.env.BASH_ENV).toBe(originalBashEnv); } + if (originalShell === undefined) { + expect(process.env.SHELL).toBeUndefined(); + } else { + expect(process.env.SHELL).toBe(originalShell); + } } }); diff --git a/src/config/config.env-vars.test.ts b/src/config/config.env-vars.test.ts index 9aba6f6db..acfbf62ad 100644 --- a/src/config/config.env-vars.test.ts +++ b/src/config/config.env-vars.test.ts @@ -30,18 +30,29 @@ describe("config env vars", () => { }); it("blocks dangerous startup env vars from config env", async () => { - await withEnvOverride({ BASH_ENV: undefined, OPENROUTER_API_KEY: undefined }, async () => { - const config = { - env: { vars: { BASH_ENV: "/tmp/pwn.sh", OPENROUTER_API_KEY: "config-key" } }, - }; - const entries = collectConfigRuntimeEnvVars(config as OpenClawConfig); - expect(entries.BASH_ENV).toBeUndefined(); - expect(entries.OPENROUTER_API_KEY).toBe("config-key"); + await withEnvOverride( + { BASH_ENV: undefined, SHELL: undefined, OPENROUTER_API_KEY: undefined }, + async () => { + const config = { + env: { + vars: { + BASH_ENV: "/tmp/pwn.sh", + SHELL: "/tmp/evil-shell", + OPENROUTER_API_KEY: "config-key", + }, + }, + }; + const entries = collectConfigRuntimeEnvVars(config as OpenClawConfig); + expect(entries.BASH_ENV).toBeUndefined(); + expect(entries.SHELL).toBeUndefined(); + expect(entries.OPENROUTER_API_KEY).toBe("config-key"); - applyConfigEnvVars(config as OpenClawConfig); - expect(process.env.BASH_ENV).toBeUndefined(); - expect(process.env.OPENROUTER_API_KEY).toBe("config-key"); - }); + applyConfigEnvVars(config as OpenClawConfig); + expect(process.env.BASH_ENV).toBeUndefined(); + expect(process.env.SHELL).toBeUndefined(); + expect(process.env.OPENROUTER_API_KEY).toBe("config-key"); + }, + ); }); it("drops non-portable env keys from config env", async () => { diff --git a/src/infra/host-env-security-policy.json b/src/infra/host-env-security-policy.json index b7760800b..aeb8200ec 100644 --- a/src/infra/host-env-security-policy.json +++ b/src/infra/host-env-security-policy.json @@ -10,6 +10,7 @@ "RUBYOPT", "BASH_ENV", "ENV", + "SHELL", "GCONV_PATH", "IFS", "SSLKEYLOGFILE" diff --git a/src/infra/host-env-security.test.ts b/src/infra/host-env-security.test.ts index 773b27dde..aefd6cd40 100644 --- a/src/infra/host-env-security.test.ts +++ b/src/infra/host-env-security.test.ts @@ -9,6 +9,7 @@ describe("isDangerousHostEnvVarName", () => { it("matches dangerous keys and prefixes case-insensitively", () => { expect(isDangerousHostEnvVarName("BASH_ENV")).toBe(true); expect(isDangerousHostEnvVarName("bash_env")).toBe(true); + expect(isDangerousHostEnvVarName("SHELL")).toBe(true); expect(isDangerousHostEnvVarName("DYLD_INSERT_LIBRARIES")).toBe(true); expect(isDangerousHostEnvVarName("ld_preload")).toBe(true); expect(isDangerousHostEnvVarName("BASH_FUNC_echo%%")).toBe(true); diff --git a/src/infra/shell-env.test.ts b/src/infra/shell-env.test.ts index 4fcb41b53..3c443a5c4 100644 --- a/src/infra/shell-env.test.ts +++ b/src/infra/shell-env.test.ts @@ -121,6 +121,38 @@ describe("shell env fallback", () => { expect(exec).toHaveBeenCalledOnce(); }); + it("falls back to /bin/sh when SHELL is non-absolute", () => { + const env: NodeJS.ProcessEnv = { SHELL: "zsh" }; + const exec = vi.fn(() => Buffer.from("OPENAI_API_KEY=from-shell\0")); + + const res = loadShellEnvFallback({ + enabled: true, + env, + expectedKeys: ["OPENAI_API_KEY"], + exec: exec as unknown as Parameters[0]["exec"], + }); + + expect(res.ok).toBe(true); + expect(exec).toHaveBeenCalledTimes(1); + expect(exec).toHaveBeenCalledWith("/bin/sh", ["-l", "-c", "env -0"], expect.any(Object)); + }); + + it("falls back to /bin/sh when SHELL points to an untrusted path", () => { + const env: NodeJS.ProcessEnv = { SHELL: "/tmp/evil-shell" }; + const exec = vi.fn(() => Buffer.from("OPENAI_API_KEY=from-shell\0")); + + const res = loadShellEnvFallback({ + enabled: true, + env, + expectedKeys: ["OPENAI_API_KEY"], + exec: exec as unknown as Parameters[0]["exec"], + }); + + expect(res.ok).toBe(true); + expect(exec).toHaveBeenCalledTimes(1); + expect(exec).toHaveBeenCalledWith("/bin/sh", ["-l", "-c", "env -0"], expect.any(Object)); + }); + it("returns null without invoking shell on win32", () => { resetShellPathCacheForTests(); const exec = vi.fn(() => Buffer.from("PATH=/usr/local/bin:/usr/bin\0HOME=/tmp\0")); diff --git a/src/infra/shell-env.ts b/src/infra/shell-env.ts index 51839c66e..0c752ce66 100644 --- a/src/infra/shell-env.ts +++ b/src/infra/shell-env.ts @@ -1,10 +1,21 @@ import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; import { isTruthyEnvValue } from "./env.js"; const DEFAULT_TIMEOUT_MS = 15_000; const DEFAULT_MAX_BUFFER_BYTES = 2 * 1024 * 1024; +const DEFAULT_SHELL = "/bin/sh"; +const TRUSTED_SHELL_PREFIXES = [ + "/bin/", + "/usr/bin/", + "/usr/local/bin/", + "/opt/homebrew/bin/", + "/run/current-system/sw/bin/", +]; let lastAppliedKeys: string[] = []; let cachedShellPath: string | null | undefined; +let cachedEtcShells: Set | null | undefined; function resolveTimeoutMs(timeoutMs: number | undefined): number { if (typeof timeoutMs !== "number" || !Number.isFinite(timeoutMs)) { @@ -13,9 +24,57 @@ function resolveTimeoutMs(timeoutMs: number | undefined): number { return Math.max(0, timeoutMs); } +function readEtcShells(): Set | null { + if (cachedEtcShells !== undefined) { + return cachedEtcShells; + } + try { + const raw = fs.readFileSync("/etc/shells", "utf8"); + const entries = raw + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith("#") && path.isAbsolute(line)); + cachedEtcShells = new Set(entries); + } catch { + cachedEtcShells = null; + } + return cachedEtcShells; +} + +function isTrustedShellPath(shell: string): boolean { + if (!path.isAbsolute(shell)) { + return false; + } + const normalized = path.normalize(shell); + if (normalized !== shell) { + return false; + } + + // Primary trust anchor: shell registered in /etc/shells. + const registeredShells = readEtcShells(); + if (registeredShells?.has(shell)) { + return true; + } + + // Fallback for environments where /etc/shells is incomplete/unavailable. + if (!TRUSTED_SHELL_PREFIXES.some((prefix) => shell.startsWith(prefix))) { + return false; + } + + try { + fs.accessSync(shell, fs.constants.X_OK); + return true; + } catch { + return false; + } +} + function resolveShell(env: NodeJS.ProcessEnv): string { const shell = env.SHELL?.trim(); - return shell && shell.length > 0 ? shell : "/bin/sh"; + if (shell && isTrustedShellPath(shell)) { + return shell; + } + return DEFAULT_SHELL; } function execLoginShellEnvZero(params: { @@ -171,6 +230,7 @@ export function getShellPathFromLoginShell(opts: { export function resetShellPathCacheForTests(): void { cachedShellPath = undefined; + cachedEtcShells = undefined; } export function getShellEnvAppliedKeys(): string[] {