mirror of
https://github.com/moltbot/moltbot.git
synced 2026-03-07 22:44:16 +00:00
fix(security): harden approval-bound node exec cwd handling
This commit is contained in:
@@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai
|
||||
- Models/Auth probes: map permanent auth failover reasons (`auth_permanent`, for example revoked keys) into probe auth status instead of `unknown`, so `openclaw models status --probe` reports actionable auth failures. (#25754) thanks @rrenamed.
|
||||
- Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
|
||||
- Security/Exec approvals: bind `system.run` approval matching to exact argv identity and preserve argv whitespace in rendered command text, preventing trailing-space executable path swaps from reusing a mismatched approval. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
|
||||
- Security/Exec approvals: harden approval-bound `system.run` execution on node hosts by rejecting symlink `cwd` paths and canonicalizing path-like executable argv before spawn, blocking mutable-cwd symlink retarget chains between approval and execution. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
|
||||
- Security/Discord reactions: enforce DM policy/allowlist authorization before reaction-event system enqueue in direct messages; Discord reaction handling now also honors DM/group-DM enablement and guild `groupPolicy` channel gating to keep reaction ingress aligned with normal message preflight. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
|
||||
- Security/Slack reactions + pins: gate `reaction_*` and `pin_*` system-event enqueue through shared sender authorization so DM `dmPolicy`/`allowFrom` and channel `users` allowlists are enforced consistently for non-message ingress, with regression coverage for denied/allowed sender paths. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
|
||||
- Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting.
|
||||
|
||||
@@ -83,6 +83,11 @@ describe("nodes-cli coverage", () => {
|
||||
const getNodeInvokeCall = () =>
|
||||
callGateway.mock.calls.find((call) => call[0]?.method === "node.invoke")?.[0] as NodeInvokeCall;
|
||||
|
||||
const getApprovalRequestCall = () =>
|
||||
callGateway.mock.calls.find((call) => call[0]?.method === "exec.approval.request")?.[0] as {
|
||||
params?: Record<string, unknown>;
|
||||
};
|
||||
|
||||
const createNodesProgram = () => {
|
||||
const program = new Command();
|
||||
program.exitOverride();
|
||||
@@ -140,6 +145,8 @@ describe("nodes-cli coverage", () => {
|
||||
runId: expect.any(String),
|
||||
});
|
||||
expect(invoke?.params?.timeoutMs).toBe(5000);
|
||||
const approval = getApprovalRequestCall();
|
||||
expect(approval?.params?.["commandArgv"]).toEqual(["echo", "hi"]);
|
||||
});
|
||||
|
||||
it("invokes system.run with raw command", async () => {
|
||||
@@ -165,6 +172,8 @@ describe("nodes-cli coverage", () => {
|
||||
approvalDecision: "allow-once",
|
||||
runId: expect.any(String),
|
||||
});
|
||||
const approval = getApprovalRequestCall();
|
||||
expect(approval?.params?.["commandArgv"]).toEqual(["/bin/sh", "-lc", "echo hi"]);
|
||||
});
|
||||
|
||||
it("invokes system.notify with provided fields", async () => {
|
||||
|
||||
@@ -252,6 +252,7 @@ export function registerNodesInvokeCommands(nodes: Command) {
|
||||
{
|
||||
id: approvalId,
|
||||
command: rawCommand ?? argv.join(" "),
|
||||
commandArgv: argv,
|
||||
cwd: opts.cwd,
|
||||
nodeId,
|
||||
host: "node",
|
||||
|
||||
@@ -49,6 +49,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
preferMacAppExecHost: boolean;
|
||||
runViaResponse?: ExecHostResponse | null;
|
||||
command?: string[];
|
||||
cwd?: string;
|
||||
security?: "full" | "allowlist";
|
||||
ask?: "off" | "on-miss" | "always";
|
||||
approved?: boolean;
|
||||
@@ -70,6 +71,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
client: {} as never,
|
||||
params: {
|
||||
command: params.command ?? ["echo", "ok"],
|
||||
cwd: params.cwd,
|
||||
approved: params.approved ?? false,
|
||||
sessionKey: "agent:main:main",
|
||||
},
|
||||
@@ -214,6 +216,71 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it.runIf(process.platform !== "win32")(
|
||||
"denies approval-based execution when cwd is a symlink",
|
||||
async () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-cwd-link-"));
|
||||
const safeDir = path.join(tmp, "safe");
|
||||
const linkDir = path.join(tmp, "cwd-link");
|
||||
const script = path.join(safeDir, "run.sh");
|
||||
fs.mkdirSync(safeDir, { recursive: true });
|
||||
fs.writeFileSync(script, "#!/bin/sh\necho SAFE\n");
|
||||
fs.chmodSync(script, 0o755);
|
||||
fs.symlinkSync(safeDir, linkDir, "dir");
|
||||
try {
|
||||
const { runCommand, sendInvokeResult } = await runSystemInvoke({
|
||||
preferMacAppExecHost: false,
|
||||
command: ["./run.sh"],
|
||||
cwd: linkDir,
|
||||
approved: true,
|
||||
security: "full",
|
||||
ask: "off",
|
||||
});
|
||||
expect(runCommand).not.toHaveBeenCalled();
|
||||
expect(sendInvokeResult).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
ok: false,
|
||||
error: expect.objectContaining({
|
||||
message: expect.stringContaining("canonical cwd"),
|
||||
}),
|
||||
}),
|
||||
);
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
it("uses canonical executable path for approval-based relative command execution", async () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-cwd-real-"));
|
||||
const script = path.join(tmp, "run.sh");
|
||||
fs.writeFileSync(script, "#!/bin/sh\necho SAFE\n");
|
||||
fs.chmodSync(script, 0o755);
|
||||
try {
|
||||
const { runCommand, sendInvokeResult } = await runSystemInvoke({
|
||||
preferMacAppExecHost: false,
|
||||
command: ["./run.sh", "--flag"],
|
||||
cwd: tmp,
|
||||
approved: true,
|
||||
security: "full",
|
||||
ask: "off",
|
||||
});
|
||||
expect(runCommand).toHaveBeenCalledWith(
|
||||
[fs.realpathSync(script), "--flag"],
|
||||
fs.realpathSync(tmp),
|
||||
undefined,
|
||||
undefined,
|
||||
);
|
||||
expect(sendInvokeResult).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
ok: true,
|
||||
}),
|
||||
);
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
it("denies ./sh wrapper spoof in allowlist on-miss mode before execution", async () => {
|
||||
const marker = path.join(os.tmpdir(), `openclaw-wrapper-spoof-${process.pid}-${Date.now()}`);
|
||||
const runCommand = vi.fn(async () => {
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
import crypto from "node:crypto";
|
||||
import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import { resolveAgentConfig } from "../agents/agent-scope.js";
|
||||
import { loadConfig } from "../config/config.js";
|
||||
import type { GatewayClient } from "../gateway/client.js";
|
||||
@@ -18,6 +20,7 @@ import {
|
||||
} from "../infra/exec-approvals.js";
|
||||
import type { ExecHostRequest, ExecHostResponse, ExecHostRunResult } from "../infra/exec-host.js";
|
||||
import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js";
|
||||
import { sameFileIdentity } from "../infra/file-identity.js";
|
||||
import { sanitizeSystemRunEnvOverrides } from "../infra/host-env-security.js";
|
||||
import { resolveSystemRunCommand } from "../infra/system-run-command.js";
|
||||
import { evaluateSystemRunPolicy, resolveExecApprovalDecision } from "./exec-policy.js";
|
||||
@@ -110,6 +113,100 @@ function normalizeDeniedReason(reason: string | null | undefined): SystemRunDeni
|
||||
}
|
||||
}
|
||||
|
||||
function isPathLikeExecutableToken(value: string): boolean {
|
||||
if (!value) {
|
||||
return false;
|
||||
}
|
||||
if (value.startsWith(".") || value.startsWith("/") || value.startsWith("\\")) {
|
||||
return true;
|
||||
}
|
||||
if (value.includes("/") || value.includes("\\")) {
|
||||
return true;
|
||||
}
|
||||
if (process.platform === "win32" && /^[a-zA-Z]:[\\/]/.test(value)) {
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
function hardenApprovedExecutionPaths(params: {
|
||||
approvedByAsk: boolean;
|
||||
argv: string[];
|
||||
shellCommand: string | null;
|
||||
cwd: string | undefined;
|
||||
}): { ok: true; argv: string[]; cwd: string | undefined } | { ok: false; message: string } {
|
||||
if (!params.approvedByAsk) {
|
||||
return { ok: true, argv: params.argv, cwd: params.cwd };
|
||||
}
|
||||
|
||||
let hardenedCwd = params.cwd;
|
||||
if (hardenedCwd) {
|
||||
const requestedCwd = path.resolve(hardenedCwd);
|
||||
let cwdLstat: fs.Stats;
|
||||
let cwdStat: fs.Stats;
|
||||
let cwdReal: string;
|
||||
let cwdRealStat: fs.Stats;
|
||||
try {
|
||||
cwdLstat = fs.lstatSync(requestedCwd);
|
||||
cwdStat = fs.statSync(requestedCwd);
|
||||
cwdReal = fs.realpathSync(requestedCwd);
|
||||
cwdRealStat = fs.statSync(cwdReal);
|
||||
} catch {
|
||||
return {
|
||||
ok: false,
|
||||
message: "SYSTEM_RUN_DENIED: approval requires an existing canonical cwd",
|
||||
};
|
||||
}
|
||||
if (!cwdStat.isDirectory()) {
|
||||
return {
|
||||
ok: false,
|
||||
message: "SYSTEM_RUN_DENIED: approval requires cwd to be a directory",
|
||||
};
|
||||
}
|
||||
if (cwdLstat.isSymbolicLink()) {
|
||||
return {
|
||||
ok: false,
|
||||
message: "SYSTEM_RUN_DENIED: approval requires canonical cwd (no symlink cwd)",
|
||||
};
|
||||
}
|
||||
if (
|
||||
!sameFileIdentity(cwdStat, cwdLstat) ||
|
||||
!sameFileIdentity(cwdStat, cwdRealStat) ||
|
||||
!sameFileIdentity(cwdLstat, cwdRealStat)
|
||||
) {
|
||||
return {
|
||||
ok: false,
|
||||
message: "SYSTEM_RUN_DENIED: approval cwd identity mismatch",
|
||||
};
|
||||
}
|
||||
hardenedCwd = cwdReal;
|
||||
}
|
||||
|
||||
if (params.shellCommand !== null || params.argv.length === 0) {
|
||||
return { ok: true, argv: params.argv, cwd: hardenedCwd };
|
||||
}
|
||||
|
||||
const argv = [...params.argv];
|
||||
const rawExecutable = argv[0] ?? "";
|
||||
if (!isPathLikeExecutableToken(rawExecutable)) {
|
||||
return { ok: true, argv, cwd: hardenedCwd };
|
||||
}
|
||||
|
||||
const base = hardenedCwd ?? process.cwd();
|
||||
const candidate = path.isAbsolute(rawExecutable)
|
||||
? rawExecutable
|
||||
: path.resolve(base, rawExecutable);
|
||||
try {
|
||||
argv[0] = fs.realpathSync(candidate);
|
||||
} catch {
|
||||
return {
|
||||
ok: false,
|
||||
message: "SYSTEM_RUN_DENIED: approval requires a stable executable path",
|
||||
};
|
||||
}
|
||||
return { ok: true, argv, cwd: hardenedCwd };
|
||||
}
|
||||
|
||||
export type HandleSystemRunInvokeOptions = {
|
||||
client: GatewayClient;
|
||||
params: SystemRunParams;
|
||||
@@ -422,6 +519,20 @@ async function evaluateSystemRunPolicyPhase(
|
||||
return null;
|
||||
}
|
||||
|
||||
const hardenedPaths = hardenApprovedExecutionPaths({
|
||||
approvedByAsk: policy.approvedByAsk,
|
||||
argv: parsed.argv,
|
||||
shellCommand: parsed.shellCommand,
|
||||
cwd: parsed.cwd,
|
||||
});
|
||||
if (!hardenedPaths.ok) {
|
||||
await sendSystemRunDenied(opts, parsed.execution, {
|
||||
reason: "approval-required",
|
||||
message: hardenedPaths.message,
|
||||
});
|
||||
return null;
|
||||
}
|
||||
|
||||
const plannedAllowlistArgv = resolvePlannedAllowlistArgv({
|
||||
security,
|
||||
shellCommand: parsed.shellCommand,
|
||||
@@ -437,6 +548,8 @@ async function evaluateSystemRunPolicyPhase(
|
||||
}
|
||||
return {
|
||||
...parsed,
|
||||
argv: hardenedPaths.argv,
|
||||
cwd: hardenedPaths.cwd,
|
||||
approvals,
|
||||
security,
|
||||
policy,
|
||||
|
||||
Reference in New Issue
Block a user