From 39dc92efb76a664b52c3ce31bcd1bda81cb4e4bb Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 18 Jun 2026 00:37:57 +0200 Subject: [PATCH] fix(security): kill timed out exec process trees --- src/process/child-process-tree.ts | 17 ++++++++++ src/secrets/resolve.test.ts | 29 ++++++++++++++++ src/secrets/resolve.ts | 11 +++++-- src/security/install-policy.test.ts | 42 ++++++++++++++++++++++++ src/security/install-policy.ts | 11 +++++-- src/test-utils/process-tree.ts | 51 +++++++++++++++++++++++++++++ 6 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 src/process/child-process-tree.ts create mode 100644 src/test-utils/process-tree.ts diff --git a/src/process/child-process-tree.ts b/src/process/child-process-tree.ts new file mode 100644 index 00000000000..00675a036f4 --- /dev/null +++ b/src/process/child-process-tree.ts @@ -0,0 +1,17 @@ +import type { ChildProcess } from "node:child_process"; +import { signalProcessTree } from "./kill-tree.js"; + +export function shouldDetachChildForProcessTree(): boolean { + return process.platform !== "win32"; +} + +export function forceKillChildProcessTree(child: Pick): void { + if (typeof child.pid === "number" && child.pid > 0) { + signalProcessTree(child.pid, "SIGKILL", { + detached: shouldDetachChildForProcessTree(), + }); + return; + } + + child.kill("SIGKILL"); +} diff --git a/src/secrets/resolve.test.ts b/src/secrets/resolve.test.ts index d264ef8905d..a6d6cefb972 100644 --- a/src/secrets/resolve.test.ts +++ b/src/secrets/resolve.test.ts @@ -5,6 +5,12 @@ import path from "node:path"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import { MAX_TIMER_TIMEOUT_MS } from "../shared/number-coercion.js"; +import { + killPidIfAlive, + readPidFile, + waitForPidToExit, + writeForkingNoOutputScript, +} from "../test-utils/process-tree.js"; import { INVALID_EXEC_SECRET_REF_IDS } from "../test-utils/secret-ref-test-vectors.js"; import { withMockedWindowsPlatform } from "../test-utils/vitest-spies.js"; import { @@ -55,6 +61,7 @@ describe("secret ref resolver", () => { jsonOnly?: boolean; allowSymlinkCommand?: boolean; trustedDirs?: string[]; + env?: Record; args?: string[]; timeoutMs?: number; noOutputTimeoutMs?: number; @@ -255,6 +262,28 @@ describe("secret ref resolver", () => { expect(value).toBe("ok"); }); + itPosix("kills forked exec provider children on no-output timeout", async () => { + const root = await createCaseDir("exec-fork-timeout"); + const scriptPath = await writeForkingNoOutputScript(root); + const pidPath = path.join(root, "forked.pid"); + let childPid: number | undefined; + + try { + await expect( + resolveExecSecret(scriptPath, { + env: { NODE_BINARY: process.execPath, PID_FILE: pidPath }, + noOutputTimeoutMs: 150, + timeoutMs: 2000, + }), + ).rejects.toThrow('Exec provider "execmain" produced no output'); + + childPid = await readPidFile(pidPath); + expect(await waitForPidToExit(childPid)).toBe(true); + } finally { + killPidIfAlive(childPid); + } + }); + itPosix("supports non-JSON single-value exec output when jsonOnly is false", async () => { const value = await resolveExecSecret(execPlainScriptPath, { jsonOnly: false }); expect(value).toBe("plain-secret"); diff --git a/src/secrets/resolve.ts b/src/secrets/resolve.ts index 8866c5eb4ba..ffda3f5f275 100644 --- a/src/secrets/resolve.ts +++ b/src/secrets/resolve.ts @@ -18,6 +18,10 @@ import { loadPluginManifestRegistry, type PluginManifestRegistry, } from "../plugins/manifest-registry.js"; +import { + forceKillChildProcessTree, + shouldDetachChildForProcessTree, +} from "../process/child-process-tree.js"; import { inspectPathPermissions, safeStat } from "../security/audit-fs.js"; import { isPathInside } from "../security/scan-paths.js"; import { resolveUserPath } from "../utils.js"; @@ -497,6 +501,7 @@ async function runExecResolver(params: { stdio: ["pipe", "pipe", "pipe"], shell: false, windowsHide: true, + detached: shouldDetachChildForProcessTree(), }); let settled = false; @@ -508,7 +513,7 @@ async function runExecResolver(params: { let noOutputTimer: NodeJS.Timeout | null = null; const timeoutTimer = setTimeout(() => { timedOut = true; - child.kill("SIGKILL"); + forceKillChildProcessTree(child); }, params.timeoutMs); const clearTimers = () => { @@ -525,7 +530,7 @@ async function runExecResolver(params: { } noOutputTimer = setTimeout(() => { noOutputTimedOut = true; - child.kill("SIGKILL"); + forceKillChildProcessTree(child); }, params.noOutputTimeoutMs); }; @@ -533,7 +538,7 @@ async function runExecResolver(params: { const text = typeof chunk === "string" ? chunk : chunk.toString("utf8"); outputBytes += Buffer.byteLength(text, "utf8"); if (outputBytes > params.maxOutputBytes) { - child.kill("SIGKILL"); + forceKillChildProcessTree(child); if (!settled) { settled = true; clearTimers(); diff --git a/src/security/install-policy.test.ts b/src/security/install-policy.test.ts index 54263f3f3be..944d82b608c 100644 --- a/src/security/install-policy.test.ts +++ b/src/security/install-policy.test.ts @@ -4,6 +4,12 @@ import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { + killPidIfAlive, + readPidFile, + waitForPidToExit, + writeForkingNoOutputScript, +} from "../test-utils/process-tree.js"; import { runInstallPolicy, validateInstallPolicyStatic, @@ -211,6 +217,42 @@ describe("runInstallPolicy", () => { expect(result).toEqual({}); }); + it.runIf(process.platform !== "win32")( + "kills forked policy command children on no-output timeout", + async () => { + const forkScriptPath = await writeForkingNoOutputScript(sourceDir); + const pidPath = path.join(sourceDir, "forked.pid"); + let childPid: number | undefined; + + try { + const result = await runInstallPolicy({ + config: { + security: { + installPolicy: { + enabled: true, + exec: { + source: "exec", + command: forkScriptPath, + env: { NODE_BINARY: process.execPath, PID_FILE: pidPath }, + allowInsecurePath: true, + noOutputTimeoutMs: 150, + timeoutMs: 2000, + }, + }, + }, + }, + request: baseRequest(sourceDir), + }); + + expect(result?.blocked?.reason).toContain("policy command produced no output"); + childPid = await readPidFile(pidPath); + expect(await waitForPidToExit(childPid)).toBe(true); + } finally { + killPidIfAlive(childPid); + } + }, + ); + it("does not inherit PATH unless passEnv includes it", async () => { const envPath = path.join(sourceDir, "env.json"); const response = JSON.stringify({ protocolVersion: 1, decision: "allow" }); diff --git a/src/security/install-policy.ts b/src/security/install-policy.ts index 8c9cf95e7d4..6519f9d7e01 100644 --- a/src/security/install-policy.ts +++ b/src/security/install-policy.ts @@ -4,6 +4,10 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { OpenClawConfig, SecurityConfig } from "../config/types.openclaw.js"; import { formatErrorMessage } from "../infra/errors.js"; +import { + forceKillChildProcessTree, + shouldDetachChildForProcessTree, +} from "../process/child-process-tree.js"; import { normalizePositiveInt, normalizePositiveTimerMs } from "../secrets/shared.js"; import { resolveUserPath } from "../utils.js"; import { resolveRuntimeServiceVersion } from "../version.js"; @@ -534,6 +538,7 @@ async function runPolicyCommand(params: { stdio: ["pipe", "pipe", "pipe"], shell: false, windowsHide: true, + detached: shouldDetachChildForProcessTree(), }); let settled = false; @@ -545,7 +550,7 @@ async function runPolicyCommand(params: { let noOutputTimer: NodeJS.Timeout | null = null; const timeoutTimer = setTimeout(() => { timedOut = true; - child.kill("SIGKILL"); + forceKillChildProcessTree(child); }, params.timeoutMs); const clearTimers = () => { @@ -562,7 +567,7 @@ async function runPolicyCommand(params: { } noOutputTimer = setTimeout(() => { noOutputTimedOut = true; - child.kill("SIGKILL"); + forceKillChildProcessTree(child); }, params.noOutputTimeoutMs); }; @@ -570,7 +575,7 @@ async function runPolicyCommand(params: { const text = typeof chunk === "string" ? chunk : chunk.toString("utf8"); outputBytes += Buffer.byteLength(text, "utf8"); if (outputBytes > params.maxOutputBytes) { - child.kill("SIGKILL"); + forceKillChildProcessTree(child); if (!settled) { settled = true; clearTimers(); diff --git a/src/test-utils/process-tree.ts b/src/test-utils/process-tree.ts new file mode 100644 index 00000000000..cda36e3a4c2 --- /dev/null +++ b/src/test-utils/process-tree.ts @@ -0,0 +1,51 @@ +import fs from "node:fs/promises"; +import path from "node:path"; + +export async function writeForkingNoOutputScript(dir: string): Promise { + const scriptPath = path.join(dir, "fork-no-output.sh"); + await fs.writeFile( + scriptPath, + [ + "#!/bin/sh", + '"$NODE_BINARY" -e "setInterval(() => {}, 1000)" &', + 'printf "%s" "$!" > "$PID_FILE"', + "sleep 30", + ].join("\n"), + "utf8", + ); + await fs.chmod(scriptPath, 0o700); + return scriptPath; +} + +export function isPidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +export async function waitForPidToExit(pid: number, timeoutMs = 2000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (!isPidAlive(pid)) { + return true; + } + await new Promise((resolve) => { + setTimeout(resolve, 25); + }); + } + return !isPidAlive(pid); +} + +export async function readPidFile(pidPath: string): Promise { + return Number((await fs.readFile(pidPath, "utf8")).trim()); +} + +export function killPidIfAlive(pid: number | undefined): void { + if (pid === undefined || !isPidAlive(pid)) { + return; + } + process.kill(pid, "SIGKILL"); +}