mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-19 07:17:26 +00:00
fix(security): harden shell env fallback
This commit is contained in:
@@ -36,6 +36,7 @@ Docs: https://docs.openclaw.ai
|
|||||||
|
|
||||||
### Fixes
|
### 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.
|
- 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/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.
|
- 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.
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ enum HostEnvSanitizer {
|
|||||||
"RUBYOPT",
|
"RUBYOPT",
|
||||||
"BASH_ENV",
|
"BASH_ENV",
|
||||||
"ENV",
|
"ENV",
|
||||||
|
"SHELL",
|
||||||
"GCONV_PATH",
|
"GCONV_PATH",
|
||||||
"IFS",
|
"IFS",
|
||||||
"SSLKEYLOGFILE",
|
"SSLKEYLOGFILE",
|
||||||
|
|||||||
@@ -360,7 +360,7 @@ describe("applySkillEnvOverrides", () => {
|
|||||||
dir: skillDir,
|
dir: skillDir,
|
||||||
name: "dangerous-env-skill",
|
name: "dangerous-env-skill",
|
||||||
description: "Needs env",
|
description: "Needs env",
|
||||||
metadata: '{"openclaw":{"requires":{"env":["BASH_ENV"]}}}',
|
metadata: '{"openclaw":{"requires":{"env":["BASH_ENV","SHELL"]}}}',
|
||||||
});
|
});
|
||||||
|
|
||||||
const entries = loadWorkspaceSkillEntries(workspaceDir, {
|
const entries = loadWorkspaceSkillEntries(workspaceDir, {
|
||||||
@@ -368,7 +368,9 @@ describe("applySkillEnvOverrides", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
const originalBashEnv = process.env.BASH_ENV;
|
const originalBashEnv = process.env.BASH_ENV;
|
||||||
|
const originalShell = process.env.SHELL;
|
||||||
delete process.env.BASH_ENV;
|
delete process.env.BASH_ENV;
|
||||||
|
delete process.env.SHELL;
|
||||||
|
|
||||||
const restore = applySkillEnvOverrides({
|
const restore = applySkillEnvOverrides({
|
||||||
skills: entries,
|
skills: entries,
|
||||||
@@ -378,6 +380,7 @@ describe("applySkillEnvOverrides", () => {
|
|||||||
"dangerous-env-skill": {
|
"dangerous-env-skill": {
|
||||||
env: {
|
env: {
|
||||||
BASH_ENV: "/tmp/pwn.sh",
|
BASH_ENV: "/tmp/pwn.sh",
|
||||||
|
SHELL: "/tmp/evil-shell",
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
@@ -387,6 +390,7 @@ describe("applySkillEnvOverrides", () => {
|
|||||||
|
|
||||||
try {
|
try {
|
||||||
expect(process.env.BASH_ENV).toBeUndefined();
|
expect(process.env.BASH_ENV).toBeUndefined();
|
||||||
|
expect(process.env.SHELL).toBeUndefined();
|
||||||
} finally {
|
} finally {
|
||||||
restore();
|
restore();
|
||||||
if (originalBashEnv === undefined) {
|
if (originalBashEnv === undefined) {
|
||||||
@@ -394,6 +398,11 @@ describe("applySkillEnvOverrides", () => {
|
|||||||
} else {
|
} else {
|
||||||
expect(process.env.BASH_ENV).toBe(originalBashEnv);
|
expect(process.env.BASH_ENV).toBe(originalBashEnv);
|
||||||
}
|
}
|
||||||
|
if (originalShell === undefined) {
|
||||||
|
expect(process.env.SHELL).toBeUndefined();
|
||||||
|
} else {
|
||||||
|
expect(process.env.SHELL).toBe(originalShell);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -30,18 +30,29 @@ describe("config env vars", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it("blocks dangerous startup env vars from config env", async () => {
|
it("blocks dangerous startup env vars from config env", async () => {
|
||||||
await withEnvOverride({ BASH_ENV: undefined, OPENROUTER_API_KEY: undefined }, async () => {
|
await withEnvOverride(
|
||||||
const config = {
|
{ BASH_ENV: undefined, SHELL: undefined, OPENROUTER_API_KEY: undefined },
|
||||||
env: { vars: { BASH_ENV: "/tmp/pwn.sh", OPENROUTER_API_KEY: "config-key" } },
|
async () => {
|
||||||
};
|
const config = {
|
||||||
const entries = collectConfigRuntimeEnvVars(config as OpenClawConfig);
|
env: {
|
||||||
expect(entries.BASH_ENV).toBeUndefined();
|
vars: {
|
||||||
expect(entries.OPENROUTER_API_KEY).toBe("config-key");
|
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);
|
applyConfigEnvVars(config as OpenClawConfig);
|
||||||
expect(process.env.BASH_ENV).toBeUndefined();
|
expect(process.env.BASH_ENV).toBeUndefined();
|
||||||
expect(process.env.OPENROUTER_API_KEY).toBe("config-key");
|
expect(process.env.SHELL).toBeUndefined();
|
||||||
});
|
expect(process.env.OPENROUTER_API_KEY).toBe("config-key");
|
||||||
|
},
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("drops non-portable env keys from config env", async () => {
|
it("drops non-portable env keys from config env", async () => {
|
||||||
|
|||||||
@@ -10,6 +10,7 @@
|
|||||||
"RUBYOPT",
|
"RUBYOPT",
|
||||||
"BASH_ENV",
|
"BASH_ENV",
|
||||||
"ENV",
|
"ENV",
|
||||||
|
"SHELL",
|
||||||
"GCONV_PATH",
|
"GCONV_PATH",
|
||||||
"IFS",
|
"IFS",
|
||||||
"SSLKEYLOGFILE"
|
"SSLKEYLOGFILE"
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ describe("isDangerousHostEnvVarName", () => {
|
|||||||
it("matches dangerous keys and prefixes case-insensitively", () => {
|
it("matches dangerous keys and prefixes case-insensitively", () => {
|
||||||
expect(isDangerousHostEnvVarName("BASH_ENV")).toBe(true);
|
expect(isDangerousHostEnvVarName("BASH_ENV")).toBe(true);
|
||||||
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("DYLD_INSERT_LIBRARIES")).toBe(true);
|
||||||
expect(isDangerousHostEnvVarName("ld_preload")).toBe(true);
|
expect(isDangerousHostEnvVarName("ld_preload")).toBe(true);
|
||||||
expect(isDangerousHostEnvVarName("BASH_FUNC_echo%%")).toBe(true);
|
expect(isDangerousHostEnvVarName("BASH_FUNC_echo%%")).toBe(true);
|
||||||
|
|||||||
@@ -121,6 +121,38 @@ describe("shell env fallback", () => {
|
|||||||
expect(exec).toHaveBeenCalledOnce();
|
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<typeof loadShellEnvFallback>[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<typeof loadShellEnvFallback>[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", () => {
|
it("returns null without invoking shell on win32", () => {
|
||||||
resetShellPathCacheForTests();
|
resetShellPathCacheForTests();
|
||||||
const exec = vi.fn(() => Buffer.from("PATH=/usr/local/bin:/usr/bin\0HOME=/tmp\0"));
|
const exec = vi.fn(() => Buffer.from("PATH=/usr/local/bin:/usr/bin\0HOME=/tmp\0"));
|
||||||
|
|||||||
@@ -1,10 +1,21 @@
|
|||||||
import { execFileSync } from "node:child_process";
|
import { execFileSync } from "node:child_process";
|
||||||
|
import fs from "node:fs";
|
||||||
|
import path from "node:path";
|
||||||
import { isTruthyEnvValue } from "./env.js";
|
import { isTruthyEnvValue } from "./env.js";
|
||||||
|
|
||||||
const DEFAULT_TIMEOUT_MS = 15_000;
|
const DEFAULT_TIMEOUT_MS = 15_000;
|
||||||
const DEFAULT_MAX_BUFFER_BYTES = 2 * 1024 * 1024;
|
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 lastAppliedKeys: string[] = [];
|
||||||
let cachedShellPath: string | null | undefined;
|
let cachedShellPath: string | null | undefined;
|
||||||
|
let cachedEtcShells: Set<string> | null | undefined;
|
||||||
|
|
||||||
function resolveTimeoutMs(timeoutMs: number | undefined): number {
|
function resolveTimeoutMs(timeoutMs: number | undefined): number {
|
||||||
if (typeof timeoutMs !== "number" || !Number.isFinite(timeoutMs)) {
|
if (typeof timeoutMs !== "number" || !Number.isFinite(timeoutMs)) {
|
||||||
@@ -13,9 +24,57 @@ function resolveTimeoutMs(timeoutMs: number | undefined): number {
|
|||||||
return Math.max(0, timeoutMs);
|
return Math.max(0, timeoutMs);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function readEtcShells(): Set<string> | 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 {
|
function resolveShell(env: NodeJS.ProcessEnv): string {
|
||||||
const shell = env.SHELL?.trim();
|
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: {
|
function execLoginShellEnvZero(params: {
|
||||||
@@ -171,6 +230,7 @@ export function getShellPathFromLoginShell(opts: {
|
|||||||
|
|
||||||
export function resetShellPathCacheForTests(): void {
|
export function resetShellPathCacheForTests(): void {
|
||||||
cachedShellPath = undefined;
|
cachedShellPath = undefined;
|
||||||
|
cachedEtcShells = undefined;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function getShellEnvAppliedKeys(): string[] {
|
export function getShellEnvAppliedKeys(): string[] {
|
||||||
|
|||||||
Reference in New Issue
Block a user