fix: revalidate approval cwd before system.run execution
This commit is contained in:
@@ -49,6 +49,7 @@ Docs: https://docs.openclaw.ai
|
||||
- Media understanding/audio transcription guard: skip tiny/empty audio files (<1024 bytes) before provider/CLI transcription to avoid noisy invalid-audio failures and preserve clean fallback behavior. (#8388) Thanks @Glucksberg.
|
||||
- OpenAI media capabilities: include `audio` in the OpenAI provider capability list so audio transcription models are eligible in media-understanding provider selection. (#12717) Thanks @openjay.
|
||||
- Security/Node exec approvals: preserve shell/dispatch-wrapper argv semantics during approval hardening so approved wrapper commands (for example `env sh -c ...`) cannot drift into a different runtime command shape, and add regression coverage for both approval-plan generation and approved runtime execution paths. Thanks @tdjackey for reporting.
|
||||
- Security/Node exec approvals: revalidate approval-bound `cwd` identity immediately before execution/forwarding and fail closed with an explicit denial when `cwd` drifts after approval hardening.
|
||||
- Security/ACP sandbox inheritance: enforce fail-closed runtime guardrails for `sessions_spawn` with `runtime="acp"` by rejecting ACP spawns from sandboxed requester sessions and rejecting `sandbox="require"` for ACP runtime, preventing sandbox-boundary bypass via host-side ACP initialization. (#32254) Thanks @tdjackey for reporting, and @dutifulbob for the fix.
|
||||
- Browser/Security output boundary hardening: replace check-then-rename output commits with root-bound fd-verified writes, unify install/skills canonical path-boundary checks, and add regression coverage for symlink-rebind race paths across browser output and shared fs-safe write flows. Thanks @tdjackey for reporting.
|
||||
- Security/fs-safe write hardening: make `writeFileWithinRoot` use same-directory temp writes plus atomic rename, add post-write inode/hardlink revalidation with security warnings on boundary drift, and avoid truncating existing targets when final rename fails.
|
||||
|
||||
@@ -576,6 +576,55 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("denies approval-based execution when cwd identity drifts before execution", async () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-cwd-drift-"));
|
||||
const fallback = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-cwd-drift-alt-"));
|
||||
const script = path.join(tmp, "run.sh");
|
||||
fs.writeFileSync(script, "#!/bin/sh\necho SAFE\n");
|
||||
fs.chmodSync(script, 0o755);
|
||||
const canonicalCwd = fs.realpathSync(tmp);
|
||||
const realStatSync = fs.statSync.bind(fs);
|
||||
let targetStatCalls = 0;
|
||||
const driftStat = realStatSync(fallback);
|
||||
const statSpy = vi.spyOn(fs, "statSync").mockImplementation((...args) => {
|
||||
const [target] = args;
|
||||
const resolvedTarget =
|
||||
typeof target === "string"
|
||||
? path.resolve(target)
|
||||
: Buffer.isBuffer(target)
|
||||
? path.resolve(target.toString())
|
||||
: target instanceof URL
|
||||
? path.resolve(target.pathname)
|
||||
: path.resolve(String(target));
|
||||
if (resolvedTarget === canonicalCwd) {
|
||||
targetStatCalls += 1;
|
||||
if (targetStatCalls >= 3) {
|
||||
return driftStat;
|
||||
}
|
||||
}
|
||||
return realStatSync(...args);
|
||||
});
|
||||
try {
|
||||
const { runCommand, sendInvokeResult } = await runSystemInvoke({
|
||||
preferMacAppExecHost: false,
|
||||
command: ["./run.sh"],
|
||||
cwd: tmp,
|
||||
approved: true,
|
||||
security: "full",
|
||||
ask: "off",
|
||||
});
|
||||
expect(runCommand).not.toHaveBeenCalled();
|
||||
expectInvokeErrorMessage(sendInvokeResult, {
|
||||
message: "SYSTEM_RUN_DENIED: approval cwd changed before execution",
|
||||
exact: true,
|
||||
});
|
||||
} finally {
|
||||
statSpy.mockRestore();
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
fs.rmSync(fallback, { 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,5 @@
|
||||
import crypto from "node:crypto";
|
||||
import fs from "node:fs";
|
||||
import { resolveAgentConfig } from "../agents/agent-scope.js";
|
||||
import { loadConfig } from "../config/config.js";
|
||||
import type { GatewayClient } from "../gateway/client.js";
|
||||
@@ -14,6 +15,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";
|
||||
@@ -81,9 +83,12 @@ type SystemRunPolicyPhase = SystemRunParsePhase & {
|
||||
segments: ExecCommandSegment[];
|
||||
plannedAllowlistArgv: string[] | undefined;
|
||||
isWindows: boolean;
|
||||
approvedCwdStat: fs.Stats | undefined;
|
||||
};
|
||||
|
||||
const safeBinTrustedDirWarningCache = new Set<string>();
|
||||
const APPROVAL_CWD_DRIFT_DENIED_MESSAGE =
|
||||
"SYSTEM_RUN_DENIED: approval cwd changed before execution";
|
||||
|
||||
function warnWritableTrustedDirOnce(message: string): void {
|
||||
if (safeBinTrustedDirWarningCache.has(message)) {
|
||||
@@ -107,6 +112,36 @@ function normalizeDeniedReason(reason: string | null | undefined): SystemRunDeni
|
||||
}
|
||||
}
|
||||
|
||||
function revalidateApprovedCwdBeforeExecution(
|
||||
phase: SystemRunPolicyPhase,
|
||||
): { ok: true } | { ok: false } {
|
||||
if (!phase.policy.approvedByAsk || !phase.cwd || !phase.approvedCwdStat) {
|
||||
return { ok: true };
|
||||
}
|
||||
const hardened = hardenApprovedExecutionPaths({
|
||||
approvedByAsk: true,
|
||||
argv: [],
|
||||
shellCommand: null,
|
||||
cwd: phase.cwd,
|
||||
});
|
||||
if (!hardened.ok || hardened.cwd !== phase.cwd) {
|
||||
return { ok: false };
|
||||
}
|
||||
let currentCwdStat: fs.Stats;
|
||||
try {
|
||||
currentCwdStat = fs.statSync(phase.cwd);
|
||||
} catch {
|
||||
return { ok: false };
|
||||
}
|
||||
if (!currentCwdStat.isDirectory()) {
|
||||
return { ok: false };
|
||||
}
|
||||
if (!sameFileIdentity(phase.approvedCwdStat, currentCwdStat)) {
|
||||
return { ok: false };
|
||||
}
|
||||
return { ok: true };
|
||||
}
|
||||
|
||||
export type HandleSystemRunInvokeOptions = {
|
||||
client: GatewayClient;
|
||||
params: SystemRunParams;
|
||||
@@ -299,6 +334,25 @@ async function evaluateSystemRunPolicyPhase(
|
||||
});
|
||||
return null;
|
||||
}
|
||||
let approvedCwdStat: fs.Stats | undefined;
|
||||
if (policy.approvedByAsk && hardenedPaths.cwd) {
|
||||
try {
|
||||
approvedCwdStat = fs.statSync(hardenedPaths.cwd);
|
||||
} catch {
|
||||
await sendSystemRunDenied(opts, parsed.execution, {
|
||||
reason: "approval-required",
|
||||
message: APPROVAL_CWD_DRIFT_DENIED_MESSAGE,
|
||||
});
|
||||
return null;
|
||||
}
|
||||
if (!approvedCwdStat.isDirectory()) {
|
||||
await sendSystemRunDenied(opts, parsed.execution, {
|
||||
reason: "approval-required",
|
||||
message: APPROVAL_CWD_DRIFT_DENIED_MESSAGE,
|
||||
});
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
const plannedAllowlistArgv = resolvePlannedAllowlistArgv({
|
||||
security,
|
||||
@@ -326,6 +380,7 @@ async function evaluateSystemRunPolicyPhase(
|
||||
segments,
|
||||
plannedAllowlistArgv: plannedAllowlistArgv ?? undefined,
|
||||
isWindows,
|
||||
approvedCwdStat,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -333,6 +388,18 @@ async function executeSystemRunPhase(
|
||||
opts: HandleSystemRunInvokeOptions,
|
||||
phase: SystemRunPolicyPhase,
|
||||
): Promise<void> {
|
||||
const cwdRevalidation = revalidateApprovedCwdBeforeExecution(phase);
|
||||
if (!cwdRevalidation.ok) {
|
||||
console.warn(
|
||||
`[security] system.run approval cwd drift blocked: runId=${phase.runId} cwd=${phase.cwd ?? ""}`,
|
||||
);
|
||||
await sendSystemRunDenied(opts, phase.execution, {
|
||||
reason: "approval-required",
|
||||
message: APPROVAL_CWD_DRIFT_DENIED_MESSAGE,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
const useMacAppExec = opts.preferMacAppExecHost;
|
||||
if (useMacAppExec) {
|
||||
const execRequest: ExecHostRequest = {
|
||||
|
||||
Reference in New Issue
Block a user