refactor(security): harden system.run wrapper enforcement

2026-03-21 16:41:56 +00:00 · 2026-02-24 02:17:24 +00:00
parent 5239b55c0a
commit 0026255def
2 changed files with 314 additions and 152 deletions
--- a/src/node-host/invoke-system-run.test.ts
+++ b/src/node-host/invoke-system-run.test.ts
@@ -1,3 +1,6 @@
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
 import { describe, expect, it, vi } from "vitest";
 import type { ExecHostResponse } from "../infra/exec-host.js";
 import { handleSystemRunInvoke, formatSystemRunAllowlistMissMessage } from "./invoke-system-run.js";
@@ -147,4 +150,67 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
      }),
    );
  });
+
+  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 () => {
+      fs.writeFileSync(marker, "executed");
+      return {
+        success: true,
+        stdout: "local-ok",
+        stderr: "",
+        timedOut: false,
+        truncated: false,
+        exitCode: 0,
+        error: null,
+      };
+    });
+    const sendInvokeResult = vi.fn(async () => {});
+    const sendNodeEvent = vi.fn(async () => {});
+
+    await handleSystemRunInvoke({
+      client: {} as never,
+      params: {
+        command: ["./sh", "-lc", "/bin/echo approved-only"],
+        sessionKey: "agent:main:main",
+      },
+      skillBins: {
+        current: async () => new Set<string>(),
+      },
+      execHostEnforced: false,
+      execHostFallbackAllowed: true,
+      resolveExecSecurity: () => "allowlist",
+      resolveExecAsk: () => "on-miss",
+      isCmdExeInvocation: () => false,
+      sanitizeEnv: () => undefined,
+      runCommand,
+      runViaMacAppExecHost: vi.fn(async () => null),
+      sendNodeEvent,
+      buildExecEventPayload: (payload) => payload,
+      sendInvokeResult,
+      sendExecFinishedEvent: vi.fn(async () => {}),
+      preferMacAppExecHost: false,
+    });
+
+    expect(runCommand).not.toHaveBeenCalled();
+    expect(fs.existsSync(marker)).toBe(false);
+    expect(sendNodeEvent).toHaveBeenCalledWith(
+      expect.anything(),
+      "exec.denied",
+      expect.objectContaining({ reason: "approval-required" }),
+    );
+    expect(sendInvokeResult).toHaveBeenCalledWith(
+      expect.objectContaining({
+        ok: false,
+        error: expect.objectContaining({
+          message: "SYSTEM_RUN_DENIED: approval required",
+        }),
+      }),
+    );
+    try {
+      fs.unlinkSync(marker);
+    } catch {
+      // no-op
+    }
+  });
 });
--- a/src/node-host/invoke-system-run.ts
+++ b/src/node-host/invoke-system-run.ts
@@ -32,9 +32,43 @@ type SystemRunInvokeResult = {
  payloadJSON?: string | null;
  error?: { code?: string; message?: string } | null;
 };
-export { formatSystemRunAllowlistMissMessage } from "./exec-policy.js";

-export async function handleSystemRunInvoke(opts: {
+type SystemRunDeniedReason =
+  | "security=deny"
+  | "approval-required"
+  | "allowlist-miss"
+  | "execution-plan-miss"
+  | "companion-unavailable"
+  | "permission:screenRecording";
+
+type SystemRunExecutionContext = {
+  sessionKey: string;
+  runId: string;
+  cmdText: string;
+};
+
+type SystemRunAllowlistAnalysis = {
+  analysisOk: boolean;
+  allowlistMatches: ExecAllowlistEntry[];
+  allowlistSatisfied: boolean;
+  segments: ExecCommandSegment[];
+};
+
+function normalizeDeniedReason(reason: string | null | undefined): SystemRunDeniedReason {
+  switch (reason) {
+    case "security=deny":
+    case "approval-required":
+    case "allowlist-miss":
+    case "execution-plan-miss":
+    case "companion-unavailable":
+    case "permission:screenRecording":
+      return reason;
+    default:
+      return "approval-required";
+  }
+}
+
+export type HandleSystemRunInvokeOptions = {
  client: GatewayClient;
  params: SystemRunParams;
  skillBins: SkillBinsProvider;
@@ -71,7 +105,161 @@ export async function handleSystemRunInvoke(opts: {
    };
  }) => Promise<void>;
  preferMacAppExecHost: boolean;
-}): Promise<void> {
+};
+
+async function sendSystemRunDenied(
+  opts: Pick<
+    HandleSystemRunInvokeOptions,
+    "client" | "sendNodeEvent" | "buildExecEventPayload" | "sendInvokeResult"
+  >,
+  execution: SystemRunExecutionContext,
+  params: {
+    reason: SystemRunDeniedReason;
+    message: string;
+  },
+) {
+  await opts.sendNodeEvent(
+    opts.client,
+    "exec.denied",
+    opts.buildExecEventPayload({
+      sessionKey: execution.sessionKey,
+      runId: execution.runId,
+      host: "node",
+      command: execution.cmdText,
+      reason: params.reason,
+    }),
+  );
+  await opts.sendInvokeResult({
+    ok: false,
+    error: { code: "UNAVAILABLE", message: params.message },
+  });
+}
+
+function evaluateSystemRunAllowlist(params: {
+  shellCommand: string | null;
+  argv: string[];
+  approvals: ReturnType<typeof resolveExecApprovals>;
+  security: ExecSecurity;
+  safeBins: ReturnType<typeof resolveExecSafeBinRuntimePolicy>["safeBins"];
+  safeBinProfiles: ReturnType<typeof resolveExecSafeBinRuntimePolicy>["safeBinProfiles"];
+  trustedSafeBinDirs: ReturnType<typeof resolveExecSafeBinRuntimePolicy>["trustedSafeBinDirs"];
+  cwd: string | undefined;
+  env: Record<string, string> | undefined;
+  skillBins: Set<string>;
+  autoAllowSkills: boolean;
+}): SystemRunAllowlistAnalysis {
+  if (params.shellCommand) {
+    const allowlistEval = evaluateShellAllowlist({
+      command: params.shellCommand,
+      allowlist: params.approvals.allowlist,
+      safeBins: params.safeBins,
+      safeBinProfiles: params.safeBinProfiles,
+      cwd: params.cwd,
+      env: params.env,
+      trustedSafeBinDirs: params.trustedSafeBinDirs,
+      skillBins: params.skillBins,
+      autoAllowSkills: params.autoAllowSkills,
+      platform: process.platform,
+    });
+    return {
+      analysisOk: allowlistEval.analysisOk,
+      allowlistMatches: allowlistEval.allowlistMatches,
+      allowlistSatisfied:
+        params.security === "allowlist" && allowlistEval.analysisOk
+          ? allowlistEval.allowlistSatisfied
+          : false,
+      segments: allowlistEval.segments,
+    };
+  }
+
+  const analysis = analyzeArgvCommand({ argv: params.argv, cwd: params.cwd, env: params.env });
+  const allowlistEval = evaluateExecAllowlist({
+    analysis,
+    allowlist: params.approvals.allowlist,
+    safeBins: params.safeBins,
+    safeBinProfiles: params.safeBinProfiles,
+    cwd: params.cwd,
+    trustedSafeBinDirs: params.trustedSafeBinDirs,
+    skillBins: params.skillBins,
+    autoAllowSkills: params.autoAllowSkills,
+  });
+  return {
+    analysisOk: analysis.ok,
+    allowlistMatches: allowlistEval.allowlistMatches,
+    allowlistSatisfied:
+      params.security === "allowlist" && analysis.ok ? allowlistEval.allowlistSatisfied : false,
+    segments: analysis.segments,
+  };
+}
+
+function resolvePlannedAllowlistArgv(params: {
+  security: ExecSecurity;
+  shellCommand: string | null;
+  policy: {
+    approvedByAsk: boolean;
+    analysisOk: boolean;
+    allowlistSatisfied: boolean;
+  };
+  segments: ExecCommandSegment[];
+}): string[] | undefined | null {
+  if (
+    params.security !== "allowlist" ||
+    params.policy.approvedByAsk ||
+    params.shellCommand ||
+    !params.policy.analysisOk ||
+    !params.policy.allowlistSatisfied ||
+    params.segments.length !== 1
+  ) {
+    return undefined;
+  }
+  const plannedAllowlistArgv = params.segments[0]?.resolution?.effectiveArgv;
+  return plannedAllowlistArgv && plannedAllowlistArgv.length > 0 ? plannedAllowlistArgv : null;
+}
+
+function resolveSystemRunExecArgv(params: {
+  plannedAllowlistArgv: string[] | undefined;
+  argv: string[];
+  security: ExecSecurity;
+  isWindows: boolean;
+  policy: {
+    approvedByAsk: boolean;
+    analysisOk: boolean;
+    allowlistSatisfied: boolean;
+  };
+  shellCommand: string | null;
+  segments: ExecCommandSegment[];
+}): string[] {
+  let execArgv = params.plannedAllowlistArgv ?? params.argv;
+  if (
+    params.security === "allowlist" &&
+    params.isWindows &&
+    !params.policy.approvedByAsk &&
+    params.shellCommand &&
+    params.policy.analysisOk &&
+    params.policy.allowlistSatisfied &&
+    params.segments.length === 1 &&
+    params.segments[0]?.argv.length > 0
+  ) {
+    execArgv = params.segments[0].argv;
+  }
+  return execArgv;
+}
+
+function applyOutputTruncation(result: RunResult) {
+  if (!result.truncated) {
+    return;
+  }
+  const suffix = "... (truncated)";
+  if (result.stderr.trim().length > 0) {
+    result.stderr = `${result.stderr}\n${suffix}`;
+  } else {
+    result.stdout = `${result.stdout}\n${suffix}`;
+  }
+}
+
+export { formatSystemRunAllowlistMissMessage } from "./exec-policy.js";
+
+export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): Promise<void> {
  const command = resolveSystemRunCommand({
    command: opts.params.command,
    rawCommand: opts.params.rawCommand,
@@ -111,6 +299,7 @@ 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 execution: SystemRunExecutionContext = { sessionKey, runId, cmdText };
  const approvalDecision = resolveExecApprovalDecision(opts.params.approvalDecision);
  const envOverrides = sanitizeSystemRunEnvOverrides({
    overrides: opts.params.env ?? undefined,
@@ -122,46 +311,19 @@ export async function handleSystemRunInvoke(opts: {
    local: agentExec,
  });
  const bins = autoAllowSkills ? await opts.skillBins.current() : new Set<string>();
-  let analysisOk = false;
-  let allowlistMatches: ExecAllowlistEntry[] = [];
-  let allowlistSatisfied = false;
-  let segments: ExecCommandSegment[] = [];
-  if (shellCommand) {
-    const allowlistEval = evaluateShellAllowlist({
-      command: shellCommand,
-      allowlist: approvals.allowlist,
-      safeBins,
-      safeBinProfiles,
-      cwd: opts.params.cwd ?? undefined,
-      env,
-      trustedSafeBinDirs,
-      skillBins: bins,
-      autoAllowSkills,
-      platform: process.platform,
-    });
-    analysisOk = allowlistEval.analysisOk;
-    allowlistMatches = allowlistEval.allowlistMatches;
-    allowlistSatisfied =
-      security === "allowlist" && analysisOk ? allowlistEval.allowlistSatisfied : false;
-    segments = allowlistEval.segments;
-  } else {
-    const analysis = analyzeArgvCommand({ argv, cwd: opts.params.cwd ?? undefined, env });
-    const allowlistEval = evaluateExecAllowlist({
-      analysis,
-      allowlist: approvals.allowlist,
-      safeBins,
-      safeBinProfiles,
-      cwd: opts.params.cwd ?? undefined,
-      trustedSafeBinDirs,
-      skillBins: bins,
-      autoAllowSkills,
-    });
-    analysisOk = analysis.ok;
-    allowlistMatches = allowlistEval.allowlistMatches;
-    allowlistSatisfied =
-      security === "allowlist" && analysisOk ? allowlistEval.allowlistSatisfied : false;
-    segments = analysis.segments;
-  }
+  let { analysisOk, allowlistMatches, allowlistSatisfied, segments } = evaluateSystemRunAllowlist({
+    shellCommand,
+    argv,
+    approvals,
+    security,
+    safeBins,
+    safeBinProfiles,
+    trustedSafeBinDirs,
+    cwd: opts.params.cwd ?? undefined,
+    env,
+    skillBins: bins,
+    autoAllowSkills,
+  });
  const isWindows = process.platform === "win32";
  const cmdInvocation = shellCommand
    ? opts.isCmdExeInvocation(segments[0]?.argv ?? [])
@@ -180,52 +342,34 @@ export async function handleSystemRunInvoke(opts: {
  analysisOk = policy.analysisOk;
  allowlistSatisfied = policy.allowlistSatisfied;
  if (!policy.allowed) {
-    await opts.sendNodeEvent(
-      opts.client,
-      "exec.denied",
-      opts.buildExecEventPayload({
-        sessionKey,
-        runId,
-        host: "node",
-        command: cmdText,
-        reason: policy.eventReason,
-      }),
-    );
-    await opts.sendInvokeResult({
-      ok: false,
-      error: { code: "UNAVAILABLE", message: policy.errorMessage },
+    await sendSystemRunDenied(opts, execution, {
+      reason: policy.eventReason,
+      message: policy.errorMessage,
    });
    return;
  }

-  let plannedAllowlistArgv: string[] | undefined;
-  if (
-    security === "allowlist" &&
-    !policy.approvedByAsk &&
-    !shellCommand &&
-    policy.analysisOk &&
-    policy.allowlistSatisfied &&
-    segments.length === 1
-  ) {
-    plannedAllowlistArgv = segments[0]?.resolution?.effectiveArgv;
-    if (!plannedAllowlistArgv || plannedAllowlistArgv.length === 0) {
-      await opts.sendNodeEvent(
-        opts.client,
-        "exec.denied",
-        opts.buildExecEventPayload({
-          sessionKey,
-          runId,
-          host: "node",
-          command: cmdText,
-          reason: "execution-plan-miss",
-        }),
-      );
-      await opts.sendInvokeResult({
-        ok: false,
-        error: { code: "UNAVAILABLE", message: "SYSTEM_RUN_DENIED: execution plan mismatch" },
-      });
-      return;
-    }
+  // Fail closed if policy/runtime drift re-allows unapproved shell wrappers.
+  if (security === "allowlist" && shellCommand && !policy.approvedByAsk) {
+    await sendSystemRunDenied(opts, execution, {
+      reason: "approval-required",
+      message: "SYSTEM_RUN_DENIED: approval required",
+    });
+    return;
+  }
+
+  const plannedAllowlistArgv = resolvePlannedAllowlistArgv({
+    security,
+    shellCommand,
+    policy,
+    segments,
+  });
+  if (plannedAllowlistArgv === null) {
+    await sendSystemRunDenied(opts, execution, {
+      reason: "execution-plan-miss",
+      message: "SYSTEM_RUN_DENIED: execution plan mismatch",
+    });
+    return;
  }

  const useMacAppExec = opts.preferMacAppExecHost;
@@ -244,42 +388,16 @@ export async function handleSystemRunInvoke(opts: {
    const response = await opts.runViaMacAppExecHost({ approvals, request: execRequest });
    if (!response) {
      if (opts.execHostEnforced || !opts.execHostFallbackAllowed) {
-        await opts.sendNodeEvent(
-          opts.client,
-          "exec.denied",
-          opts.buildExecEventPayload({
-            sessionKey,
-            runId,
-            host: "node",
-            command: cmdText,
-            reason: "companion-unavailable",
-          }),
-        );
-        await opts.sendInvokeResult({
-          ok: false,
-          error: {
-            code: "UNAVAILABLE",
-            message: "COMPANION_APP_UNAVAILABLE: macOS app exec host unreachable",
-          },
+        await sendSystemRunDenied(opts, execution, {
+          reason: "companion-unavailable",
+          message: "COMPANION_APP_UNAVAILABLE: macOS app exec host unreachable",
        });
        return;
      }
    } else if (!response.ok) {
-      const reason = response.error.reason ?? "approval-required";
-      await opts.sendNodeEvent(
-        opts.client,
-        "exec.denied",
-        opts.buildExecEventPayload({
-          sessionKey,
-          runId,
-          host: "node",
-          command: cmdText,
-          reason,
-        }),
-      );
-      await opts.sendInvokeResult({
-        ok: false,
-        error: { code: "UNAVAILABLE", message: response.error.message },
+      await sendSystemRunDenied(opts, execution, {
+        reason: normalizeDeniedReason(response.error.reason),
+        message: response.error.message,
      });
      return;
    } else {
@@ -327,37 +445,22 @@ export async function handleSystemRunInvoke(opts: {
  }

  if (opts.params.needsScreenRecording === true) {
-    await opts.sendNodeEvent(
-      opts.client,
-      "exec.denied",
-      opts.buildExecEventPayload({
-        sessionKey,
-        runId,
-        host: "node",
-        command: cmdText,
-        reason: "permission:screenRecording",
-      }),
-    );
-    await opts.sendInvokeResult({
-      ok: false,
-      error: { code: "UNAVAILABLE", message: "PERMISSION_MISSING: screenRecording" },
+    await sendSystemRunDenied(opts, execution, {
+      reason: "permission:screenRecording",
+      message: "PERMISSION_MISSING: screenRecording",
    });
    return;
  }

-  let execArgv = plannedAllowlistArgv ?? argv;
-  if (
-    security === "allowlist" &&
-    isWindows &&
-    !policy.approvedByAsk &&
-    shellCommand &&
-    policy.analysisOk &&
-    policy.allowlistSatisfied &&
-    segments.length === 1 &&
-    segments[0]?.argv.length > 0
-  ) {
-    execArgv = segments[0].argv;
-  }
+  const execArgv = resolveSystemRunExecArgv({
+    plannedAllowlistArgv: plannedAllowlistArgv ?? undefined,
+    argv,
+    security,
+    isWindows,
+    policy,
+    shellCommand,
+    segments,
+  });

  const result = await opts.runCommand(
    execArgv,
@@ -365,14 +468,7 @@ export async function handleSystemRunInvoke(opts: {
    env,
    opts.params.timeoutMs ?? undefined,
  );
-  if (result.truncated) {
-    const suffix = "... (truncated)";
-    if (result.stderr.trim().length > 0) {
-      result.stderr = `${result.stderr}\n${suffix}`;
-    } else {
-      result.stdout = `${result.stdout}\n${suffix}`;
-    }
-  }
+  applyOutputTruncation(result);
  await opts.sendExecFinishedEvent({ sessionKey, runId, cmdText, result });

  await opts.sendInvokeResult({